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

The new `sunpy.coordinates.sun` #3163

Merged
merged 12 commits into from May 31, 2019

Conversation

Projects
None yet
4 participants
@ayshih
Copy link
Contributor

commented May 31, 2019

As proposed in #3158, this PR gathers all functions from sunpy.sun.sun and some functions from sunpy.coordinates.ephemeris into a new sunpy.coordinates.sun. Of note:

  • Deprecation warnings abound. However, do our deprecation warnings not show by default? I have to turn them on (via warnings.simplefilter) Fixed by @Cadair
  • To avoid circular imports, I had to leave the sunpy.coordinates.ephemeris functions' code where it was. I renamed the functions to be private functions, and call them in place with deprecated hooks and call them from their future locations with their future names. It's weird, I know, but it can all be cleaned up once we're past the deprecation period.
  • I don't understand why the docs are failing (but maybe it's because of my contortions above?) Ah, the automatic docs ignore functions that have __module__ different from the current module because they look to be non-local.

Closes #3158

@sunpy-bot

This comment has been minimized.

Copy link

commented May 31, 2019

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

@pep8speaks

This comment has been minimized.

Copy link

commented May 31, 2019

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

Line 174:101: E501 line too long (109 > 100 characters)
Line 180:101: E501 line too long (107 > 100 characters)
Line 181:101: E501 line too long (109 > 100 characters)
Line 188:101: E501 line too long (109 > 100 characters)

Comment last updated at 2019-05-31 18:06:02 UTC

@ayshih ayshih force-pushed the ayshih:sun branch from 141b261 to c8f2532 May 31, 2019

@ayshih ayshih removed the [WIP] label May 31, 2019

@ayshih ayshih added this to the 1.0 milestone May 31, 2019

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

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

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

Show resolved Hide resolved sunpy/coordinates/sun.py Outdated

Cadair added some commits May 31, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 31, 2019

@ayshih well spotted on the deprecation warning front, I have fixed that in 421a2b4

@Cadair

Cadair approved these changes May 31, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Grumble, GOES is offline again and it's breaking our doc builds.

@Cadair Cadair requested a review from sunpy/sunpy-developers May 31, 2019

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

To explain, the worst circular imports were due to sunpy.coordinates.sun depending on sunpy.coordinates.ephemeris on import (to create some constants), but the deprecation hooks in sunpy.coordinates.ephemeris needing to point to the new functions in sunpy.coordinates.sun also on import. By using the admittedly weird approach with private functions, where the hooks in sunpy.coordinates.ephemeris point within the same module, the dependencies stay in one direction.

@Cadair

Cadair approved these changes May 31, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 31, 2019

The problem with the doc build here is that it's failing right on an example at the top, and then stopping, which means we have no idea if there are errors in the rest of the build 🤦‍♂

Show resolved Hide resolved sunpy/coordinates/ephemeris.py Outdated
@wafels

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Other than my bigger complaint about 'now', +1!

@ayshih

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Other than my bigger complaint about 'now', +1!

Heh, good enough!

I'll add the JPL HORIZONS web form to docstring.

@ayshih ayshih force-pushed the ayshih:sun branch from 753855a to 69ef78d May 31, 2019

@Cadair Cadair merged commit 2e1c4d4 into sunpy:master May 31, 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 98.03% of diff hit (target 90.32%)
Details
codecov/project 90.33% (+<.01%) compared to 69755cd
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190531.14 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 added a commit that referenced this pull request May 31, 2019

Merge pull request #3163 from ayshih/sun
The new `sunpy.coordinates.sun`

@ayshih ayshih deleted the ayshih:sun branch May 31, 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.