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

Fix for aiaprep - issue #1691 #1756

Merged
merged 4 commits into from
May 18, 2016
Merged

Fix for aiaprep - issue #1691 #1756

merged 4 commits into from
May 18, 2016

Conversation

larrymanley
Copy link
Contributor

Generic fix for sunpy.instr.aia.aiaprep, issue #1691. Works for full-resolution Level 1 data or data that has already been reduced.

center = np.floor(tempmap.meta['crpix1'])
leftx = center - aiamap.data.shape[0] / 2
rightx = center + aiamap.data.shape[0] / 2
newmap = tempmap.submap([leftx, rightx]*u.pix, [leftx, rightx]*u.pix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you are using just shape[0], is it because the image need to be square?
In any case, another way of doing this without repetition would be:

range_side = (center + np.array([-1, 1]) * aiamap.data.shape[0] / 2) * u.pix
newmap = tempmap.submap(range_side, range_side)

tempmap = aiamap.rotate(recenter=True, scale=scale_factor.value, missing=aiamap.min())

# extract center from padded aiamap.rotate output
center = np.floor(tempmap.meta['crpix1'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you only use crpix1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crpix1 and crpix2 will always be the same (recenter=True) - aiaprep doesn't work on submaps

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add that as a comment to clairify for future Stu?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment and docstring?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just an inline comment above the line will suffice, it's a suble implementation detail that's all.

@Cadair Cadair added map Affects the map submodule [BugFix] Affects Release An issue/bug that affects a released version (use a version label too) labels May 12, 2016
@Cadair Cadair modified the milestone: 0.7 May 12, 2016
tempmap = aiamap.rotate(recenter=True, scale=scale_factor.value, missing=aiamap.min())

# extract center from padded aiamap.rotate output
# crpix1 and crpix2 will be equal (recenter=True), as aiaprep does not work with submaps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you assure it doesn't work for a submap? I've just tried and it didn't show any error or warning that I should not being doing this. Maybe raising an error if the dimensions are not right would be better than just proceed as normal and produce bad data:

>>> saia_map = aia_map.submap(u.Quantity([30, 809], u.pix), u.Quantity([35, 789], u.pix))
>>> saia_map_prep = sunpy.instr.aia.aiaprep(saia_map)
>>> saia_map.dimensions
Pair(x=<Quantity 779.0 pix>, y=<Quantity 754.0 pix>)
>>> saia_map_prep.dimensions
Pair(x=<Quantity 754.0 pix>, y=<Quantity 741.0 pix>)
>>> saia_map.xrange     
>>> <Quantity [-1156.8,  712.8] arcsec>
>>> saia_map_prep.xrange    
<Quantity [-903.6, 906. ] arcsec>
>>> saia_map.yrange     
<Quantity [-1144.8,  664.8] arcsec>
>>> saia_map_prep.yrange
<Quantity [-873.6, 904.8] arcsec>

@Cadair
Copy link
Member

Cadair commented May 16, 2016

@larrymanley @wafels @dpshelio Can this PR be merged as is to close the obvious bug and the discussion can continue after the 0.7 feature freeze?

@Cadair
Copy link
Member

Cadair commented May 16, 2016

@larrymanley you need to merge master into this to fix the conflict.

@larrymanley
Copy link
Contributor Author

@Cadair What command do I use for that? Then a push from my local?

@Cadair
Copy link
Member

Cadair commented May 16, 2016

@larrymanley assuming you have github.com/sunpy/sunpy configured as "upstream" and github.com/larrymanley/sunpy as "origin" you can do:

git pull upstream master
git push origin master

@Cadair
Copy link
Member

Cadair commented May 18, 2016

I have opened #1783 to continue the discussion regarding submaps, but in the interests of getting a 0.7 branch cut, I am going to merge this. 👍

@Cadair Cadair merged commit 95bbe48 into sunpy:master May 18, 2016
@Cadair
Copy link
Member

Cadair commented May 18, 2016

Note to self: This needs backporting to 0.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects Release An issue/bug that affects a released version (use a version label too) map Affects the map submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants