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

Change the way observer location is calculated in Map #3115

Merged
merged 4 commits into from May 21, 2019

Conversation

Projects
5 participants
@Cadair
Copy link
Member

commented May 20, 2019

This is essentially a re-do and generalisation of #3056 because of the error @wafels has encountered in #2972

In #2972 @wafels changes the observer location in the header (to HGS) which dosen't work with an AIAMap because we now extract a different set of kwargs from the header and therefore to overwrite the location successfully we would have to write HEA coordinates to the header.

This PR changes AIAMap to default to extracting HEA but to fall back to HGS or HGC if those keys are present but not the previous ones.

This however, causes a change in behaviour of Map because it now checks for all three of the observer location keys in one go rather than checking them individually and then defaulting them individually to Earth if they can't be found. This means that if some of the header information is there and some of it isn't before this PR we could end up with an inconsistent observer location, which now wouldn't happen.

fixes #3068

Cadair added some commits May 20, 2019

@Cadair Cadair requested a review from ayshih May 20, 2019

@pep8speaks

This comment has been minimized.

Copy link

commented May 20, 2019

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

Line 570:93: E231 missing whitespace after ','

Line 73:9: E124 closing bracket does not match visual indentation

Line 165:101: E501 line too long (112 > 100 characters)
Line 183:101: E501 line too long (112 > 100 characters)
Line 188:101: E501 line too long (112 > 100 characters)
Line 193:101: E501 line too long (112 > 100 characters)

Comment last updated at 2019-05-20 10:20:34 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented May 20, 2019

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

@Cadair Cadair added this to the 1.0 milestone May 20, 2019

@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

@wafels I think what you would need to do in #2972 once this is merged is pop the HEA keys from the map meta before changing the HGS ones.

@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

I thought about it a little more and added a helper function for you @wafels

@Cadair Cadair added this to Pre-Feature Freeze (2019-05-08) in SunPy 1.0 May 20, 2019

@Cadair Cadair requested review from wafels and sunpy/sunpy-developers May 20, 2019

Cadair added a commit to Cadair/sunpy that referenced this pull request May 20, 2019

@ayshih

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

See also #3068!

@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Ah yes. This fixes that 😂🤣

heliographic_latitude = self._default_heliographic_latitude
all_keys = [str(e[0]) for e in self._supported_observer_coordinates]
all_keys = '\n'.join(all_keys)
warning_message = ("Missing metadata for observer: assuming Earth-based observer."

This comment has been minimized.

Copy link
@dstansby

dstansby May 20, 2019

Contributor

I think it would be useful to tell the user exactly which metadata is missing here (instead of just telling them that something is missing).

This comment has been minimized.

Copy link
@Cadair

Cadair May 20, 2019

Author Member

It's not trivial to work out what ones are missing, i.e. you could have a weird header with a mix of crln and hglt and it would be hard to work out which one the user was aiming for.

I will give it some thought.

This comment has been minimized.

Copy link
@hayesla

hayesla May 20, 2019

Contributor

I think improved docs on map meta will really help here

This comment has been minimized.

Copy link
@Cadair

Cadair May 21, 2019

Author Member

I opened an issue to keep track of this because I didn't want to block this on it.

@ayshih

ayshih approved these changes May 20, 2019

Copy link
Contributor

left a comment

Sweet!

@Cadair Cadair merged commit ff1563b into sunpy:master May 21, 2019

16 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 95.83% of diff hit (target 89.88%)
Details
codecov/project 89.92% (+0.03%) compared to 75985aa
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190520.6 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

@Cadair Cadair deleted the Cadair:fix_map_observer branch May 21, 2019

@Cadair Cadair moved this from Pre-Feature Freeze (2019-05-08) to Finished in SunPy 1.0 May 21, 2019

Cadair added a commit to Cadair/sunpy that referenced this pull request May 22, 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.