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.

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

Bugfix for bitmasks passed to nddata #14995

Merged
merged 2 commits into from Jul 5, 2023
Merged

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Jun 27, 2023

Description

This pull request revises one line changed in PR #14175, which gave rise to Issue #14978. I've added a test to ensure that we will notice if this happens again in the future.

Discussion

This PR adds the first test for bitmask support in ndarithmetic. We may want to discuss if in the docs and tests we should explicitly claim support for bitmasks in the NDData.mask attribute (@eteq), as well as whether astropy/astropy-APEs#14 should be revived (@perrygreenfield, @mwcraig, @jehturner @crawfordsm @MSeifert04, @parejkoj).

Fixes #14978.

cc @KathleenLabrie @chris-simpson

@github-actions
Copy link

github-actions bot commented Jun 27, 2023

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.

@bmorris3 bmorris3 added the 💤 backport-v5.3.x on-merge: backport to v5.3.x label Jun 27, 2023
@bmorris3 bmorris3 added this to the v5.3.1 milestone Jun 27, 2023
@bmorris3 bmorris3 requested a review from mwcraig June 27, 2023 19:52
@pllim pllim added the Bug label Jun 27, 2023
@jehturner
Copy link
Member

jehturner commented Jun 27, 2023

Thanks. It will take us a little while to run our tests on this (probably some time tomorrow, Wed).

@mwcraig
Copy link
Member

mwcraig commented Jun 28, 2023

This looks good to me; @bmorris3 can you please add a changelog entry? Let's wait until @jehturner reports back on his tests before merging.

@jehturner
Copy link
Member

Unfortunately there's a coincidental problem with our tests that is preventing them from running and will take a bit of unplanned maintenance, so we might not be able to confirm that this looks good today.

@bmorris3
Copy link
Contributor Author

@jehturner Do you think you'll have a chance to check by the end of the week?

@jehturner
Copy link
Member

I hope so, yes.

@bmorris3
Copy link
Contributor Author

bmorris3 commented Jul 3, 2023

@jehturner Any results from your testing?

@jehturner
Copy link
Member

Hello. I finally managed to fix our tests late on Friday night, but I tried testing this PR at the weekend and the AstroPy build failed, so I'll need to look at that first.

@jehturner
Copy link
Member

jehturner commented Jul 4, 2023

This has passed our testing with DRAGONS 3.1, apart from 1 irrelevant test. Thanks a lot!

@jehturner
Copy link
Member

BTW, one of several problems that I had when trying to install this branch was that pip believes the astropy version to be 1.0rc2.dev23962+g39310eb (which would be incompatible with other packages). Any idea why? @pllim? I have seen odd behaviour with development versions of astropy before and assume it's something to do with the code that automatically figures out the version number, but I'm not sure how it comes up with 1.0rc2 from a fresh checkout??

@WilliamJamieson
Copy link
Contributor

BTW, one of several problems that I had when trying to install this branch was that pip believes the astropy version to be 1.0rc2.dev23962+g39310eb (which would be incompatible with other packages). Any idea why? @pllim? I have seen odd behaviour with development versions of astropy before and assume it's something to do with the code that automatically figures out the version number, but I'm not sure how it comes up with 1.0rc2 from a fresh checkout??

I believe the issue here is that either you or @bmorris3 need to synchronize your tags with the main astropy repository.

@WilliamJamieson
Copy link
Contributor

I just took a look, @bmorris3 the version issue will be resolved if you pull the astropy tags into your fork.

Assuming you have added the astropy/astropy repository as a remote under upstream then you can run:

git fetch upstream --tags
git push origin --tags

This should resolve this issue in the future.

@jehturner
Copy link
Member

Thanks for confirming that, @WilliamJamieson (it sounds familiar now that you mention it). In any case, I was able to work around the problem with a hack for testing the current PR.

@bmorris3
Copy link
Contributor Author

bmorris3 commented Jul 5, 2023

Thanks @WilliamJamieson, should be all fixed now. Good for merge?

@bmorris3 bmorris3 merged commit c0702da into astropy:main Jul 5, 2023
24 of 25 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jul 5, 2023
@pllim
Copy link
Member

pllim commented Jul 5, 2023

The tag problem should be gone now if you pull astropy dev from this repo instead of PR branch. But if it still a problem, please let us know!

pllim added a commit that referenced this pull request Jul 5, 2023
…995-on-v5.3.x

Backport PR #14995 on branch v5.3.x (Bugfix for bitmasks passed to nddata)
@mwcraig
Copy link
Member

mwcraig commented Jul 5, 2023

Thanks for the quick fix @bmorris3, and thanks to @KathleenLabrie for reporting the bug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug nddata 💤 backport-v5.3.x on-merge: backport to v5.3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In v5.3, NDDataRef mask propagation fails when one of the operand does not have a mask
5 participants