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

Attempt to correctly read observer locations for MDI and EIT #3067

Merged
merged 9 commits into from May 22, 2019

Conversation

Projects
4 participants
@Cadair
Copy link
Member

commented Apr 24, 2019

Description

This is inspired by #3056 and conversations around it. This changes the SoHo sources so they do not actually modify the header, but implement the properties needed to override things.

Fixes #2430 and fixes #3040

This now builds on #3115

@Cadair Cadair requested a review from ayshih Apr 24, 2019

@sunpy-bot

This comment has been minimized.

Copy link

commented Apr 24, 2019

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

@Cadair Cadair added this to the 1.0 milestone Apr 24, 2019

@ehsteve

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

See issue #3070.

@ayshih

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Need to fix this to use HeliocentricMeanEcliptic instead of HeliocentricTrueEcliptic for Astropy 3.2 (see #3066 and #3075)

@Cadair Cadair force-pushed the Cadair:soho_observers branch from 240aaba to 66d57bc May 16, 2019

@ayshih

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Your MDI implementation doesn't fix #3040. It looks like you've put their value for Carrington longitude where the Stonyhurst longitude should go.

I believe our options are to trust their Carrington longitude (which I don't like because I can't match Stonyhurst-Carrington transformations exactly), to try to calculate a Stonyhurst longitude by subtracting two header values (which at first glance doesn't give accurate results), or to kludge it by declaring that the Stonyhurst longitude is zero and document that kludge.

@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

@ayshih I went with the convert to stonyhurst approach. It seems the least kludgy? We already actually do that in mapbase by default: https://github.com/sunpy/sunpy/blob/master/sunpy/map/mapbase.py#L606 which is maybe something we might want to consider?! (HMI uses it as they don't have HGS in the header)

@Cadair Cadair added this to Post-Feature Freeze in SunPy 1.0 May 17, 2019

@ayshih

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

I think you made the right choice, but it's an uncomfortable choice since I still don't trust HGC coordinates (see #2224). For cross-reference purposes, #3057 contains my anxiety over HMI.

@ayshih

ayshih approved these changes May 17, 2019

@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

I think this test fail might actually be real.

@Cadair Cadair force-pushed the Cadair:soho_observers branch from 7c8894e to 3a38cfc 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 80:9: E124 closing bracket does not match visual indentation
Line 195:9: E124 closing bracket does not match visual indentation

Comment last updated at 2019-05-22 13:00:48 UTC
@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

@ayshih It seems that the distance for EIT isn't coming out right by doing this, which is causing the fails in the differential rotation code.

@ayshih

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@Cadair I've pushed the fix

@Cadair

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

oh dear 🤦‍♀ oh dear oh dear oh dear.

Cadair and others added some commits Apr 24, 2019

@Cadair Cadair force-pushed the Cadair:soho_observers branch from 06cbd9b to c1bee8f May 22, 2019

@Cadair Cadair merged commit 80c2e97 into sunpy:master May 22, 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 100% of diff hit (target 90.07%)
Details
codecov/project 90.07% (+<.01%) compared to a031c23
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190522.7 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:soho_observers branch May 22, 2019

@Cadair Cadair moved this from Post-Feature Freeze to Finished in SunPy 1.0 May 22, 2019

nabobalis added a commit to nabobalis/sunpy that referenced this pull request May 22, 2019

Merge pull request sunpy#3067 from Cadair/soho_observers
Attempt to correctly read observer locations for MDI and EIT
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.