-
-
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
Fixed WCS.wcs.cunit equality #9154
Conversation
When the first item was the same the previous approach considered them equal and didn't check the following items. Also removed the comparison of unit_class since that's just the astropy.units.Unit class and shouldn't be relevant for equality. Maybe that reference should not be kept in the instances but rather as static reference but that's something that can be addressed in the future and is not appropriate to change in a bug-fix commit.
…owed in C99 mode)
Just as an aside: I found this bug because of a compiler generated warning that looked very suspicious:
And I somehow overlooked this in #8480. |
astropy/wcs/src/unit_list_proxy.c
Outdated
if (equal == -1) { | ||
return NULL; // Exception will be set because the rich-compare failed | ||
|
||
{ |
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.
That is this {
tied to? Seems to be orphaned. Am I reading this right?
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.
Yep, it's orphaned. It's a so called "nested scope" (it creates a new scope for variables). I used it here to separate the code dealing with the pre-condition checks and the actual comparison.
If that's confusing I can remove it again - it's not necessary 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.
If it is not strictly necessary, I would vote to remove it. Or at least state the intention of the block. Thanks!
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.
Okay 👍
Cc @astrofrog (original issue) |
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.
Looks good, thanks! I wonder whether this should be backported to LTS - maybe @bsipocz can advise?
On my phone currently - but i think the patch adding the eq was added in 3.2 so no need to backport? |
Thank you for the review! 👍 |
#8480 was indeed milestoned for 3.2. Thanks! |
@pllim I wish you could give me a chance to review before my merging WCS PRs. |
@nden , I deeply apologize and I take full responsibility! |
@nden In case you have some comments or questions regarding the changes I'll still answer or address them. Just let me know :) |
@MSeifert04 I'm sure you will :) . I just want to be aware of what is happening in the package without feeling I'm in a race. Thanks for the contributions! |
When the first item was the same the previous approach considered them equal and didn't check the following items.
Also removed the comparison of
unit_class
since that's just theastropy.units.Unit
class and shouldn't be relevant for equality. Maybe that reference should not be kept in the instances but rather as static reference but that's something that can be addressed in the future and is not appropriate to change in a bug-fix commit.