Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add atol argument to function is_O3 #14371

Merged
merged 1 commit into from Feb 9, 2023
Merged

Add atol argument to function is_O3 #14371

merged 1 commit into from Feb 9, 2023

Conversation

cmarmo
Copy link
Member

@cmarmo cmarmo commented Feb 8, 2023

Description

This pull request is to address the addition of the atol argument to the function is_O3.
Related test is added.

Fixes #13694

Thank you for considering it.

@github-actions
Copy link

github-actions bot commented Feb 8, 2023 •
edited by pllim

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v5.3 milestone Feb 8, 2023
@pllim pllim requested a review from nstarman February 8, 2023 19:35
@pllim
Copy link
Member

pllim commented Feb 8, 2023

Thanks! I need this needs a change log. 😸

@nstarman
Copy link
Member

nstarman commented Feb 8, 2023

Thanks @cmarmo! This looks very good. It would be good to add atol to is_rotation so that it can be passed to is_O3.

@mhvk, is_O3 is used in UnitSphericalRepresentation.transform. It would be nice to be able set atol there as well, but if this is an argument then it breaks Liskov substitution. Any ideas? configuration variable? hm...

@cmarmo
Copy link
Member Author

cmarmo commented Feb 8, 2023

It would be good to add atol to is_rotation so that it can be passed to is_O3.

Should I introduce atol in np.isclose in the is_rotation function?
I need some more minutes to figure out the test needed for my last commit... this originates my question. :)
Thanks!

@mhvk
Copy link
Contributor

mhvk commented Feb 8, 2023

My tendency would be not to propagate it, since all it really provides in UnitSpherical is a short-cut for just transforming and then doing .represent_as(UnitSpherical).

It would be nice, though, for the default atol to depend on the dtype of the matrix. E.g., have a default of atol=None, and then

if atol is None:
    atol = np.finfo(matrix.dtype).eps * 5

(factor 5 to reproduce ~1e-15 for float64)

@nstarman
Copy link
Member

nstarman commented Feb 8, 2023

I think that's a good change! And this will help with transform as well, since it is dtype dependent.

nstarman
nstarman reviewed Feb 8, 2023
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmarmo, I think this is along the lines @mhvk is suggesting. The same should be applied to is_rotation and documented in their docstrings. Thanks for the quick edits, this will be nice to get in!

astropy/coordinates/matrix_utilities.py Outdated
@@ -136,14 +136,18 @@ def angle_axis(matrix):
return Angle(angle, u.radian), -axis / r


def is_O3(matrix):
def is_O3(matrix, atol=1e-15):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def is_O3(matrix, atol=1e-15):
def is_O3(matrix, atol=None):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nstarman, I changed a bit your suggestion, as one of the tests uses an int64 matrix and finfo was giving me the numpy error

ValueError: data type <class 'numpy.int64'> not inexact

Once fixed it, the value of atol for integers was None and that was giving me another numpy error

TypeError: unsupported operand type(s) for +: 'NoneType' and 'float'

I hope the current version reproduces the expected behavior...

astropy/coordinates/matrix_utilities.py Show resolved Hide resolved
nstarman
nstarman reviewed Feb 8, 2023
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cmarmo. This is getting close. The new additions aren't completely covered by the test suite, which should be an easy fix.

astropy/coordinates/matrix_utilities.py Outdated Show resolved Hide resolved
astropy/coordinates/matrix_utilities.py Outdated Show resolved Hide resolved
nstarman
nstarman reviewed Feb 9, 2023
View reviewed changes
astropy/coordinates/matrix_utilities.py Outdated
Comment on lines 168 to 169
if isinstance(matrix.dtype, np.floating):
atol = np.finfo(matrix.dtype).eps * 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a small mistake here. The dtype can't be checked this way because the dtype is a class, not an instance. The way to check the dtype is with issubdtype. This should fix the test issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask you why this fixed the test? Also , is this line still useful?

astropy/astropy/coordinates/tests/test_matrix_utilities.py

Line 107 in 6002fea

assert tuple(is_rotation(n1, atol=None)) == (True, True) # (show the broadcasting)

Sorry I'm a bit thick...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that float64 is not an instance of a subclass of floating.

@cmarmo
Copy link
Member Author

cmarmo commented Feb 9, 2023 •
edited

I have just learned how to use coverage locally... :) ... I will manage to fix the test...
Next time...

nstarman
nstarman approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cmarmo for the PR and fast turnaround. Last request, please squash down the number of commits to 1 to 3. Thanks!
Also let's get the approval of one of the coordinates maintainers!

@cmarmo @nstarman
Add atol argument to function is_O3 and is_rotation
20ab22e 
Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
mhvk
mhvk approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good! Thanks, @cmarmo!

@mhvk mhvk merged commit 24a4ee1 into astropy:main Feb 9, 2023
@cmarmo cmarmo deleted the is-o3-atol branch February 9, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk approved these changes

@nstarman nstarman nstarman approved these changes

@eerovaher eerovaher Awaiting requested review from eerovaher

Assignees
No one assigned
Labels
coordinates
Projects
None yet
Milestone
v5.3
Development

Successfully merging this pull request may close these issues.

Add atol argument to function is_O3
4 participants
@cmarmo @pllim @nstarman @mhvk