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

Re-implementation of `sunpy.sun.sun` functions #3137

Merged
merged 10 commits into from May 27, 2019

Conversation

Projects
3 participants
@ayshih
Copy link
Contributor

commented May 26, 2019

I have re-implemented the sunpy.sun.sun functions using the coordinates framework and Astropy's underlying machinery (ERFA). Of note:

  • Accuracy is now much improved, and can be compared against published values (e.g., Astronomical Almanac) to the <0.1-arcsecond level
  • Functions to calculate intermediate products (e.g., mean anomaly) have been deleted since they are non-trivial to re-implement, and folks aren't typically interested in the intermediate products
  • Re-defines the output of some functions (position() and true_obliquity_of_ecliptic()), so definitely a breaking PR, even if the above functions were preserved
  • The conversion of ecliptic to equatorial coordinates is explicitly coded. In principle, the coordinates framework could be used, but the precise frames needed do not (yet) exist in Astropy.
  • Curiously, the previous implementations of true_declination() and apparent_declination() were each bugged, not just inaccurate.
  • The RA/dec functions default to providing equinox-of-date values, but have the option to provide J2000.0 values instead.

Closes #472 and closes #3021

@ayshih ayshih added the coordinates label May 26, 2019

@ayshih ayshih requested review from dpshelio and Cadair May 26, 2019

@sunpy-bot

This comment has been minimized.

Copy link

commented May 26, 2019

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

@Cadair

Cadair approved these changes May 27, 2019

ra = true_rightascension(t)
dec = true_declination(t)
ra = apparent_rightascension(t)
dec = apparent_declination(t)
return ra, dec

This comment has been minimized.

Copy link
@Cadair

Cadair May 27, 2019

Member

Should this return a SkyCoord?

This comment has been minimized.

Copy link
@ayshih

ayshih May 27, 2019

Author Contributor

In principle, yes, but the appropriate frame doesn't currently exist in Astropy. The frame needs to be PrecessedGeocentric plus nutation corrections (i.e., true equinox of date instead of mean equinox of date). So, for now, a tuple is what we can do.

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

@sunpy sunpy deleted a comment from sunpy-bot bot May 27, 2019

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

@nabobalis
Copy link
Contributor

left a comment

Code looks good, I can't vouch for the math tho.

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Now even more awesome. I realized that I can easily use the coordinates framework to get J2000.0 RA/declination, so I added that in (by setting the boolean keyword equinox_of_date to False).

@Cadair Cadair merged commit 9695ccd into sunpy:master May 27, 2019

14 of 16 checks passed

sunpy.sunpy Build #20190527.18 had test failures
Details
sunpy.sunpy (Linux_37_online) Linux_37_online failed
Details
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 97.93% of diff hit (target 90.27%)
Details
codecov/project 90.29% (+0.01%) compared to c07a396
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
@Cadair

This comment has been minimized.

Copy link
Member

commented May 27, 2019

Thanks @ayshih this is awesome 😄

@ayshih ayshih deleted the ayshih:sun branch May 28, 2019

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

Merge pull request sunpy#3137 from ayshih/sun
Re-implementation of `sunpy.sun.sun` functions
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Backported.

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

Merge pull request sunpy#3137 from ayshih/sun
Re-implementation of `sunpy.sun.sun` functions

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

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

Merge pull request sunpy#3137 from ayshih/sun
Re-implementation of `sunpy.sun.sun` functions
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.