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

Port enhancements to bitfield from stsci.tools #611

Merged
merged 4 commits into from Apr 5, 2018

Conversation

mcara
Copy link
Contributor

@mcara mcara commented Jan 31, 2018

This PR enhances the treatment of very high bit flags in such a way that flags like 1<<63 (maximum supported by uint64 type) now work and flags larger than maximum supported by numpy.uint64 are ignored and again allow code to work without crashing. See #610 for more details.

@mwcraig
Copy link
Member

mwcraig commented Feb 2, 2018

Excellent, thanks for this @mcara! I'll take a look at it this Sunday.

@mcara
Copy link
Contributor Author

mcara commented Feb 2, 2018

@mwcraig Thanks! You may also want to take a look at my tests https://github.com/spacetelescope/stsci.tools/blob/master/lib/stsci/tools/tests/test_bitmask.py They are somewhat different from your tests for bitfield and provide 100% coverage of non-deprecated code stsci.tools.bitmask module. They also include these edge-case tests for "extreme" bit flags. However, you may not like how I designed them (i.e., parametrization, naming, etc.).

@mwcraig
Copy link
Member

mwcraig commented Feb 5, 2018

@mcara -- this looks good; I'm trying (and so far failing) to restart the tests....

I thought your tests looked great! Are you suggesting we add those here too? I'm ok either way.

Actually, broader question, which doesn't have to be answered here: would you be interested in migrating the bitfield stuff into astropy.nddata at some point?

@mwcraig
Copy link
Member

mwcraig commented Feb 5, 2018

closing/reopening to try to trigger a new build of the tests...

@mwcraig mwcraig closed this Feb 5, 2018
@mwcraig mwcraig reopened this Feb 5, 2018
@mcara
Copy link
Contributor Author

mcara commented Feb 5, 2018

With regard to tests, I suggest several things and really it is up to you (it's your package) what you choose (in no particular preference from my side):

  1. Replace your tests with my tests "as is".
  2. Add my tests "as is".
  3. You can cherrypick what what you want and make a PR yourself (or someone from your group) and rename/re-format my code.
  4. I can make a separate PR for tests only an maybe you can fork it and modify it to you liking (naming conventions, notations, formatting, etc.) to do either 1. or 2. above.
  5. I can add my tests to this PR to do either 1. & 2.

@mcara
Copy link
Contributor Author

mcara commented Feb 5, 2018

@mwcraig I hope I fixed the problem in the CHANGES.rst so that at least one failing test will pass. I do not know why the other tests are failing.

@bsipocz
Copy link
Member

bsipocz commented Feb 5, 2018

The current test failure is unrelated and is due to pytest being upgraded as part of installing mpl to a version that astropy 1.0.x doesn't support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants