-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
Update dimensions metadata when resampling a map #2010
Conversation
Hello @prateekiiest! Thanks for updating the PR.
Comment last updated on February 28, 2017 at 04:15 Hours UTC |
sunpy/map/mapbase.py
Outdated
@@ -847,6 +847,8 @@ def resample(self, dimensions, method='linear'): | |||
new_meta['CD2_2'] *= scale_factor_y | |||
new_meta['crpix1'] = (dimensions[0].value + 1) / 2. | |||
new_meta['crpix2'] = (dimensions[1].value + 1) / 2. | |||
new_meta['naxis1'] = dimensions[0] |
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.
FITS headers have the axes reversed, so naxis1 = dimensions[1]
also it should be dimensions.value
so it's a float.
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.
changed the keys
@@ -847,6 +847,8 @@ def resample(self, dimensions, method='linear'): | |||
new_meta['CD2_2'] *= scale_factor_y | |||
new_meta['crpix1'] = (dimensions[0].value + 1) / 2. | |||
new_meta['crpix2'] = (dimensions[1].value + 1) / 2. | |||
new_meta['naxis1'] = dimensions[1].value |
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.
you should not add these keys if they do not exist in the original metadata.
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.
I have send a new PR for the test case for Generic Map #2011 .
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.
Generic Map testing one??
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.
def test_resample_metadata(generic_map, sample_method, new_dimensions):
"""
Check that the resampled map has correctly adjusted metadata.
"""
resampled_map = generic_map.resample(new_dimensions, method=sample_method)
assert float(resampled_map.meta['cdelt1']) / generic_map.meta['cdelt1']
== float(generic_map.data.shape[1]) / resampled_map.data.shape[1]
assert float(resampled_map.meta['cdelt2']) / generic_map.meta['cdelt2']
== float(generic_map.data.shape[0]) / resampled_map.data.shape[0]
assert resampled_map.meta['crpix1'] == (resampled_map.data.shape[1] + 1) / 2.
assert resampled_map.meta['crpix2'] == (resampled_map.data.shape[0] + 1) / 2.
assert resampled_map.meta['crval1'] == generic_map.center.x.value
assert resampled_map.meta['crval2'] == generic_map.center.y.value
for key in generic_map.meta:
if key not in ('cdelt1', 'cdelt2', 'crpix1', 'crpix2',
'crval1', 'crval2'):
assert resampled_map.meta[key] == generic_map.meta[key]
Actually in this part the (if key is not) didn't include the naxis keys. So I just added this naxis1 and naxis2 in that section
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.
My point is that this test should not need these keys. There is no need for us to add the naxis
keys to the header, only update them if they are already there.
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.
Ok , got it.
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.
Cleared pep8 issues.
But could not solve issues regarding exceeding 79 characters in one line.
Issue #1870
I updated the 'naxis' values in the resampled map.