Rotation model split#2269
Conversation
suggested a number of times. This changes MatrixRotation2D to simply Rotation2D with a single parameter: angle (currently given in degrees--Quantity support will allow some more flexibility here). Its inverse is to simply apply a rotation by the same amount in the opposite direction. Added a simple test case for this because why not?
…f the old MatrixRotation2D where it could take an arbitrary 2D matrix. This specifically limits the matrix to 2x2 (hence 2D). This could easily be extended to be n-D but is currently just implementing what might have actually worked with the old MatrixRotation2D, with the addition of an additional 'translation' parameter which takes a translation vector, allowing the full range of affine transformations. Includes a couple simple tests.
…the sense or rotatioin is in fact counter-clockwise
|
Oops, worth pointing out that the first half of this--the rotation model--was also done in #2149. I think the original version of that PR didn't have it, so I didn't realize that it was since updated with this feature. There it's called |
There was a problem hiding this comment.
Start with a one-line description to get nicer API docs listing:
Perform an affine transformation in 2 dimensions.
|
I've added some inline comments with suggestions to improve the API docs for these classes a bit. |
|
Should this be a |
|
@nden I was thinking that too--there's no reason it couldn't be. I just wasn't sure if that was desired or not. I'll make the change. |
…eed to be maintained for them in case they are changed/removed later.
|
@cdeil I think I've addressed all your comments. I have some plans in mind for improving the per-model doc pages in general, but for now this should bring the docs more or less in line for these models. @nden I opened #2277 to track making this model fittable. It occurred to me as soon as I started working on it that in order for this to work we'll need the better support for array-like parameters in general. |
|
👍 to merge this. |
As discussed in #2149 and elsewhere, this removes the
MatrixRotation2Dmodel and replaces it with two new models:Rotation2D(essentially the same as the oldMatrixRotation2D, but only accepts anangleparameter) andAffineTransformation2Dwhich replaces the use ofMatrixRotation2Dwith an arbitrary matrix, and also adds simple translations. This also incorporates/supersedes #2266 such that the sense of rotation used byRotation2Dis counter-clockwise.A few notes on
AffineTranformation2D:astropy.modeling.projectionswhich seemed like the most appropriate place for now, but I'm not sure. I'm open to other suggestions.MatrixRotation2Dseemed to demand.Rotation2Dcould be implemented aroundAffineTransformation2D. The latter could also, eventually, be implemented as a composite model. But for the time being it's more straightforward to have a concrete implementation.