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

Updated stpyfits to replace deprecated function NumCode #36

Merged
merged 1 commit into from May 19, 2017

Conversation

Projects
None yet
3 participants
@stsci-hack
Contributor

stsci-hack commented May 19, 2017

This simple update to stpyfits addresses Issue #35 by replacing the call to io.fits.NumCode with the call to the BITPIX2DTYPE dict instead.

Manual testing indicates this resolves the problem identified during pipeline testing. However, this needs to be reviewed by someone familiar with io.fits ( @jhunkeler ?) or someone who uses it for pipeline support ( @mcara ) before being merged into master.

@pllim

This comment has been minimized.

Contributor

pllim commented May 19, 2017

Something does not look quite right, you did not import BITPIX2DTYPE from within astropy.io.fits.

Note: Also see astropy/astropy#4993

@stsci-hack

This comment has been minimized.

Contributor

stsci-hack commented May 19, 2017

This change works due to line 314 where the code uses 'from...import *'. (That was a design decision made years ago... I am just continuing to take advantage of it!).

@pllim

This comment has been minimized.

Contributor

pllim commented May 19, 2017

In that case, LGTM.

@stsci-hack stsci-hack merged commit 0c1d498 into spacetelescope:master May 19, 2017

@stsci-hack stsci-hack deleted the stsci-hack:replace_depr_numcode branch May 19, 2017

@mcara

This comment has been minimized.

Member

mcara commented on lib/stsci/tools/stpyfits.py in 1e94ffd May 23, 2017

Why not do the following:

    code = BITPIX2DTYPE[bitpix] if hasattr(fits, 'BITPIX2DTYPE') else self.NumCode[bitpix]

?
This would be backward compatible.
CC: @jhunkeler

This comment has been minimized.

Contributor

pllim replied May 23, 2017

Backward compatible with what? I think Warren's change would support the min version of Astropy that we require anyway.

This comment has been minimized.

Member

mcara replied May 23, 2017

I think Warren's change would support the min version of Astropy that we require anyway.

In that case just disregard my comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment