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

Fix for allowing WCS to take Image HDU #4505

Merged
merged 2 commits into from May 10, 2016

Conversation

Projects
None yet
6 participants
@anchitjain1234
Contributor

anchitjain1234 commented Jan 19, 2016

For #4493.
Do I need to add tests for it?

Show outdated Hide outdated astropy/wcs/wcs.py
@@ -389,6 +389,9 @@ def __init__(self, header=None, fobj=None, key=' ', minerr=0.0,
header_string = header
elif isinstance(header, fits.Header):
header_string = header.tostring().rstrip()
elif type(header) == fits.ImageHDU or type(header) == fits.PrimaryHDU:

This comment has been minimized.

@embray

embray Jan 19, 2016

Member

This should be written isinstance(header, (fits.ImageHDU, fits.PrimaryHDU)), or you can kill two birds with one stone by going with their common base class (which may be broader anyways): isinstance(header, fits.hdu.image._ImageBaseHDU).

@embray

embray Jan 19, 2016

Member

This should be written isinstance(header, (fits.ImageHDU, fits.PrimaryHDU)), or you can kill two birds with one stone by going with their common base class (which may be broader anyways): isinstance(header, fits.hdu.image._ImageBaseHDU).

Show outdated Hide outdated astropy/wcs/wcs.py
@@ -389,6 +389,9 @@ def __init__(self, header=None, fobj=None, key=' ', minerr=0.0,
header_string = header
elif isinstance(header, fits.Header):
header_string = header.tostring().rstrip()
elif type(header) == fits.ImageHDU or type(header) == fits.PrimaryHDU:
header = header.header
header_string = header.tostring().rstrip()

This comment has been minimized.

@embray

embray Jan 19, 2016

Member

It occurs to me that this line is repeated at the end of every branch in this conditional. It should just be moved after the if/else. The last clause in the conditional does create a new variable new_header, but it could just as easily write

orig_header = header
header = fits.Header()
@embray

embray Jan 19, 2016

Member

It occurs to me that this line is repeated at the end of every branch in this conditional. It should just be moved after the if/else. The last clause in the conditional does create a new variable new_header, but it could just as easily write

orig_header = header
header = fits.Header()

This comment has been minimized.

@anchitjain1234

anchitjain1234 Jan 19, 2016

Contributor

But in first clause of this conditional when is_path is False than header_string = header which cant be resolved by moving header_string = header.tostring().rstrip() to last.
How to resolve that?

@anchitjain1234

anchitjain1234 Jan 19, 2016

Contributor

But in first clause of this conditional when is_path is False than header_string = header which cant be resolved by moving header_string = header.tostring().rstrip() to last.
How to resolve that?

This comment has been minimized.

@embray

embray Jan 19, 2016

Member

Ah I missed that. Maybe something at the end like

if isinstance(header, fits.Header):
    header_string = header.tostring().rstrip()
else:
    header_string = header
@embray

embray Jan 19, 2016

Member

Ah I missed that. Maybe something at the end like

if isinstance(header, fits.Header):
    header_string = header.tostring().rstrip()
else:
    header_string = header
@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Jan 20, 2016

Contributor

@embray Where to put changelog entry?
And what is the general rule for where to update in CHANGES.rst?

Contributor

anchitjain1234 commented Jan 20, 2016

@embray Where to put changelog entry?
And what is the general rule for where to update in CHANGES.rst?

@embray

This comment has been minimized.

Show comment
Hide comment
@embray

embray Jan 20, 2016

Member

Wait until a maintainer assigns a milestone.

Member

embray commented Jan 20, 2016

Wait until a maintainer assigns a milestone.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Jan 22, 2016

Contributor

@embray Changes are fine now?

Contributor

anchitjain1234 commented Jan 22, 2016

@embray Changes are fine now?

Show outdated Hide outdated astropy/wcs/tests/test_wcs.py
path = get_pkg_data_filename(path)
hdulist = fits.open(path)
w=wcs.WCS(hdulist[0])
w.printwcs()

This comment has been minimized.

@nden

nden Jan 26, 2016

Contributor

These are essentially two identical tests. I would suggest using only one file, e.g. extensions 0 and 1 of validate.fits and not using sip2.fits. Also would it make sense not to print the wcs but to compare two objects - one initialized with an HDU and the other initialized by passing the corresponding header? For example something like:

# these three should be the same
w = WCS(f[0])
w1 = WCS(f[0].header)
w = WCS()
assert w.wcs.compare(w1)
@nden

nden Jan 26, 2016

Contributor

These are essentially two identical tests. I would suggest using only one file, e.g. extensions 0 and 1 of validate.fits and not using sip2.fits. Also would it make sense not to print the wcs but to compare two objects - one initialized with an HDU and the other initialized by passing the corresponding header? For example something like:

# these three should be the same
w = WCS(f[0])
w1 = WCS(f[0].header)
w = WCS()
assert w.wcs.compare(w1)

@nden nden added the wcs label Jan 26, 2016

@nden nden added this to the v1.2.0 milestone Jan 26, 2016

@nden

This comment has been minimized.

Show comment
Hide comment
@nden

nden Jan 26, 2016

Contributor

@astrofrog I set this to 1.2 and I don't think it needs affects-release as it is new behaviour but feel free to change it if this is not correct.

Contributor

nden commented Jan 26, 2016

@astrofrog I set this to 1.2 and I don't think it needs affects-release as it is new behaviour but feel free to change it if this is not correct.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Jan 26, 2016

Contributor

Does this look OK @embray @astrofrog?

Contributor

anchitjain1234 commented Jan 26, 2016

Does this look OK @embray @astrofrog?

Show outdated Hide outdated CHANGES.rst
@@ -213,6 +213,8 @@ Bug fixes
- ``astropy.wcs``
- Fix for allowing WCS to take Image HDU. [#4493]

This comment has been minimized.

@embray

embray Jan 27, 2016

Member

This is an API change. Just write "WCS objects can now be initialized with an ImageHDU or PrimaryHDU object. [#4493]"

@embray

embray Jan 27, 2016

Member

This is an API change. Just write "WCS objects can now be initialized with an ImageHDU or PrimaryHDU object. [#4493]"

@embray

This comment has been minimized.

Show comment
Hide comment
@embray

embray Jan 27, 2016

Member

Other than the changelog entry looks fine, thanks. I would also do a rebase to squash down to one commit, but that's just my preference--not really important.

Member

embray commented Jan 27, 2016

Other than the changelog entry looks fine, thanks. I would also do a rebase to squash down to one commit, but that's just my preference--not really important.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Jan 27, 2016

Contributor

@embray Rebased and squashed to one commit.

Contributor

anchitjain1234 commented Jan 27, 2016

@embray Rebased and squashed to one commit.

@embray

This comment has been minimized.

Show comment
Hide comment
@embray

embray Jan 27, 2016

Member

@anchitjain1234 Looks great, thanks. Though I'm afraid there's one more thing I just noticed missing from this. I should have noticed sooner. This docstring needs to be updated to reflect the expanded interface: https://github.com/astropy/astropy/pull/4505/files#diff-d4c5b9e74c6c26000684075497033504R218

Member

embray commented Jan 27, 2016

@anchitjain1234 Looks great, thanks. Though I'm afraid there's one more thing I just noticed missing from this. I should have noticed sooner. This docstring needs to be updated to reflect the expanded interface: https://github.com/astropy/astropy/pull/4505/files#diff-d4c5b9e74c6c26000684075497033504R218

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Jan 31, 2016

Contributor

@embray could you please check changes?

Contributor

anchitjain1234 commented Jan 31, 2016

@embray could you please check changes?

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 4, 2016

Contributor

@astrofrog @embray could you please check the changes?

Contributor

anchitjain1234 commented Feb 4, 2016

@astrofrog @embray could you please check the changes?

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Feb 8, 2016

Contributor

@embray please check the changes.

Contributor

anchitjain1234 commented Feb 8, 2016

@embray please check the changes.

@bsipocz

This comment has been minimized.

Show comment
Hide comment
@bsipocz

bsipocz Feb 8, 2016

Member

@anchitjain1234 - Please note that the system sends out notifications to everyone watching the repo (~130 people), so there is absolutely no need to have multiple pings per PRs. This is already milestoned, and I'm sure someone will be back in due time.

Member

bsipocz commented Feb 8, 2016

@anchitjain1234 - Please note that the system sends out notifications to everyone watching the repo (~130 people), so there is absolutely no need to have multiple pings per PRs. This is already milestoned, and I'm sure someone will be back in due time.

@embray

This comment has been minimized.

Show comment
Hide comment
@embray

embray Feb 18, 2016

Member

What @bsipocz said.

Member

embray commented Feb 18, 2016

What @bsipocz said.

@eteq eteq removed the Affects-release label Apr 5, 2016

@eteq

This comment has been minimized.

Show comment
Hide comment
@eteq

eteq Apr 5, 2016

Member

looks like this needs a rebase now...? other than that I think it's good, assuming the tests pass post-rebase.

Member

eteq commented Apr 5, 2016

looks like this needs a rebase now...? other than that I think it's good, assuming the tests pass post-rebase.

@anchitjain1234

This comment has been minimized.

Show comment
Hide comment
@anchitjain1234

anchitjain1234 Apr 5, 2016

Contributor

@eteq Rebased.

Contributor

anchitjain1234 commented Apr 5, 2016

@eteq Rebased.

@astrofrog

This comment has been minimized.

Show comment
Hide comment
@astrofrog
Member

astrofrog commented May 10, 2016

Thanks @anchitjain1234!

@astrofrog astrofrog merged commit 078f01e into astropy:master May 10, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 76.592%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment