-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Planar2d model #5456
Add Planar2d model #5456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -67,6 +67,8 @@ New Features | |||
- Added the injection of EntryPoints into astropy.modeling.fitting if | |||
they inherit from Fitters class. [#5241] | |||
|
|||
- Added ``Planar2D`` functional model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to include this PR number in the change log. See other entries for formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, thanks.
|
||
See Also | ||
-------- | ||
Linear1D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also include Polynomial2D here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can include Polynomial2D here, yes.
@@ -786,6 +786,53 @@ def inverse(self): | |||
return self.__class__(slope=new_slope, intercept=new_intercept) | |||
|
|||
|
|||
class Planar2D(Fittable2DModel): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just a special case of Polynomial2D
? So, should it really subclass Polynomial2D
and belong in polynomial.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, why isn't the same done with Linear1D? It's in the same spirit as that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I dig back at the history at notice that Linear1D
was added pretty early via #1203 as part of some other refactoring. So probably could have but did not happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @nden can comment more on this. If subclassing Polynomial2D has no other added benefits, then this is probably okay as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's simpler if it does not inherit from Polynomial2D
which does more computations because it's general.
This looks good to me! 👍 on merging when tests pass. |
Tests passed. I will let someone else merge because if it's up to me, I would have asked for squashing... |
@pllim - what's "squashing"? Also, do I need to do anything about the changelog conflict? |
@gkanarek You need to resolve the conflict in
An editor window will open with a list of all commits. Mark the ones you want to squash with Sounds like a lot but it's not that bad. See also the help for rebase. |
0e3329c
to
73ab0d3
Compare
You can use a 1st-degree Polynomial2D as a plane, but this is intuitive and easy to recognize.
73ab0d3
to
e48e781
Compare
I rebased and squashed this PR. I'll merge when tests pass. |
Oh thank you - I hadn't had a chance to do this yet! |
No problem. This helped me to finally figure out how to use the new "maintainer take over" GitHub feature. 😄 |
While one can use a 1st-degree Polynomial2D to represent a plane, there should be a Planar2D functional model to parallel the Linear1D functional model.