Change "Reprojecting Images to Different Observers" example to avoid using custom wcs header until necessary #6853
Conversation
| # properties to do it. | ||
| # Due to a mismatch between the solar radius as defined in ``map_aia`` | ||
| # and ``map_euvi``, our reprojection will not behave correctly on pixels | ||
| # very near the limb. This can be prevented by equating the two radii. |
There was a problem hiding this comment.
| # very near the limb. This can be prevented by equating the two radii. | |
| # very near the limb. This can be prevented by assuming that the emission | |
| # observed by EUVI was emitted at the same radius from the centre of the Sun | |
| # as the emission observed by AIA. |
There was a problem hiding this comment.
I think this is a bit clearer on exactly what the line of code means in terms of assumption
There was a problem hiding this comment.
I agree that it is clearer as to what an assumption about rsun means in general but I think there might be a danger there that readers leave with the idea that the mismatch between EUVI (or really the sunpy default rsun value) and AIA rsun is because of different emission heights.
There was a problem hiding this comment.
To make it all clear, how about:
Data providers can set the radius at which emission in the map is assumed to have come from. Most maps set this to the standard photospheric radius (including EUI maps), but some maps (including AIA maps) set it to a slightly larger value. This mismatch means our reprojection won't work correctly on pixels near the limb. This can be prevented by assuming that the emission observed by EUVI was emitted at the same radius from the centre of the Sun as the emission observed by AIA.
? I think it's not a bad thing if users go away understanding that emission can come from different heights, and there has to be an assumption about those heights somewhere?
There was a problem hiding this comment.
I like this paragraph, I think I was being stupid: my thought was this needs an explanation page for what rsun is, how it can have different values and why that can cause problems with reproject.
Now I have realised that this text can go in here now and when the page is made, be removed or reduced in place of a link, will make the change.
|
Instead of suggesting to a user to modify the EUVI metadata ( map_aia.wcs.wcs.aux.rsun_ref = sunpy.sun.constants.radius.to_value('m') |
alasdairwilson
force-pushed
the
simplify_reproject_eg
branch
from
9d37682 to
eb76521
Compare
What's the motivation for doing this instead of modifying the metadata? Because |
|
Removing this from 5.0 milestone, we can backport this to a bugfix release if it doesn't get in 5.0.0 |
So I agree, and without any opposing argument: I am going to revert this change and the surrounding text to represent that change. In addition to your point, it makes sense because it is not a generic change, if the maps were reversed then setting the map_eui metadata to the default value would not fix it (since it is already the default sunpy value) whereas equating the two meta values will always work. |
|
Something went wrong ... Please have a look at my logs. It seems that the branch you are trying to backport to does not exist. |
This changes the Reprojecting Images to Different Observers example to avoid making a custom wcs until necessary (doesn't use it for euvi but does for mars).
I think there has been repeated confusion amongst users that see this example and assume a custom output wcs is necessary.
To avoid the reprojected image having NaNs on the limb, we still force the rsun for the two maps to match before reprojecting.
This however only partly deals with #6585 , I think a diataxis style explanation page is still needed, this example is now begging for a: "for a full explanation of why this is necessary go here" kind of deal but I am not sure I am the man for that page. I can definitely start it if people want, get the ball rolling (in a new PR,) but will probably need help from more savy reprojecters to weigh in.