-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Allow Ellipse2D and Sersic2D theta to be a quantity #13030
Conversation
I'm the one who knocks! (A special day message.) |
27fee73
to
daba2aa
Compare
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 only see one small changed to the tests (the fix for the bounding box test on Ellipse2D
).
Can you add tests for Ellipse2D
and Sersic2D
with theta
acting both as a "float as radian" and as a "angle quality"?
I've added unit tests. |
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.
The tests look good for the most part. My final request is to add an evaluation comparison to each one (as I have suggested my comments). This way we know they models are consistent with each other and that they properly evaluate using both theta formulations.
Yes, that sounds good. I've added the model evaluation tests. Thanks! |
The CI failures are unrelated and are coming from VOTable. |
Looks related to me. You forgot to mark
|
e502157
to
483188a
Compare
Thanks, @pllim . Indeed, |
Description
This PR allows the
Ellipse2D
andSersic2D
theta
parameter to be input as an angular quantity. I'm calling this a new "feature" instead of a "bugfix" (but I can change that). Technically, there was nothing preventing quantitytheta
inputs, but thetheta
docstrings forEllipse2D
andSersic2D
only mention "float as radian". Also, forEllipse2D
a quantitytheta
would give the incorrectbounding_box
(there was actually anEllipse2D
unit test for quantity theta, but it gives the wrong result).Sersic2D
does not define abounding_box
, so angulartheta
works as expected, but was not documented.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CI
label.no-changelog-entry-needed
label. If this is a manual backport, use theskip-changelog-checks
label unless special changelog handling is necessary.astropy-bot
check might be missing; do not let the green checkmark fool you.backport-X.Y.x
label(s) before merge.