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

Don't assume arcseconds when CUNIT metadata isn't present #2847

Merged
merged 2 commits into from Mar 16, 2019

Conversation

Projects
None yet
6 participants
@dstansby
Copy link
Contributor

commented Nov 19, 2018

Blindly assuming that data is in units of arcseconds seems like a Bad Idea. A concrete example is LASCO C3 data that doesn't have any metadata (see #2775 (comment) and comments below). Currently LASCO C3 data with missing metadata is assumed to be in arcseconds, which is completely wrong and results in e.g. the draw-solar-limb function putting the solar limb in completely the wrong place without even a warning.

At the very least I think assuming arcseconds when metadata is missing should be done on an instrument-by-instrument level, and not in mapbase. This PR removes the arcsecod assumption in mapbase.

@pep8speaks

This comment has been minimized.

Copy link

commented Nov 19, 2018

Hello @dstansby! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-03-16 16:22:13 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Nov 19, 2018

Thanks for the pull request @dstansby! Everything looks great!

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch 3 times, most recently from fc131b0 to 8d49a08 Nov 23, 2018

Show resolved Hide resolved sunpy/map/mapbase.py Outdated

@nabobalis nabobalis added this to the 1.0 milestone Jan 7, 2019

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch from 0027102 to 0df28f0 Jan 13, 2019

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

This is kind of all working, but I'm not really sure what to do about the EIT sample map that's missing the metadata?

Show resolved Hide resolved sunpy/map/mapbase.py Outdated
Show resolved Hide resolved sunpy/map/sources/soho.py Outdated

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch from fa0e1f0 to 8f23cfe Feb 5, 2019

@dstansby

This comment was marked as outdated.

Copy link
Contributor Author

commented Feb 5, 2019

wooops, force pushed an old version, so will have to dig the correct new version out from my laptop.

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch 2 times, most recently from f5fa6ee to d800da1 Feb 5, 2019

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

🎉 it works!

Show resolved Hide resolved sunpy/map/mapbase.py Outdated
@ehsteve

ehsteve approved these changes Mar 6, 2019

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch 2 times, most recently from 9b58b3e to 5254f9c Mar 7, 2019

Show resolved Hide resolved sunpy/map/mapbase.py Outdated
@@ -37,6 +37,9 @@ class SJIMap(GenericMap):
"""

def __init__(self, data, header, **kwargs):
# Assume pixel units are arcesc if not given

This comment has been minimized.

Copy link
@Cadair

Cadair Mar 8, 2019

Member

is this still needed with the change to MapFactory?

This comment has been minimized.

Copy link
@dstansby

dstansby Mar 14, 2019

Author Contributor

Not sure; I'll double check all these statements I've added.

@Cadair

Cadair approved these changes Mar 8, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@dstansby one of the online test fails is real.

Show resolved Hide resolved sunpy/map/mapbase.py Outdated

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch from 1466c2e to d1279d3 Mar 9, 2019

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

So it seems like we need to provide users for instructions on how to fix metadata before creating a Map from a .fits file with missing cunit metadata. Does anyone know a simple way to do this?

Don't assume arcseconds when CUNIT metadata isn't present
Raise a more sensible Attribute error if CUNIT metadata not present

Fix mapbase tests

Only set LASCO cunit if present

Fix maps doctests

Add changelog

Fix map_from_numpy_array

Fix maps_example

Add clearer error message when unit meatadata not present

Assume arcseconds for EIT maps

Update error message

Move cunit check to _validate_metadata

Revert change to LASCOmap

Add custom Validation error

Fix tests

Add instructions on fixing missing metadata

Update missing metadata warning

Add missing doctest skips

Fix doctests

@dstansby dstansby force-pushed the dstansby:no-assume-arcsec branch from 1f812dd to 9f04eb2 Mar 14, 2019

@@ -0,0 +1,2 @@
Maps no longer assume that the pixel units are arcseconds if the units aren't
explicitly set.

This comment has been minimized.

Copy link
@Cadair

Cadair Mar 14, 2019

Member

Can you add a comment here that they will fail on instantiation now as well?

Out of date

@Cadair Cadair merged commit ade2115 into sunpy:master Mar 16, 2019

7 checks passed

sunpy-bot All checks passed
sunpy.sunpy Build #20190316.13 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details

@dstansby dstansby deleted the dstansby:no-assume-arcsec branch Mar 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.