-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
Implemented ACE Clients in the current version of SunPy #3705
Conversation
Hello @abhijeetmanhas! 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 2020-02-05 14:15:17 UTC |
Line too long errors are because in the ace file, the examples how to use within comments are of Fido queries, like below in a single line it can be ignored or reduced by using |
You are gonna have to workout what parts of those files you need to port into the current version of those files using some sort of merge tool (meld for example). |
1416b40
to
12f3cb2
Compare
I will check the master version of all functions imported imported in |
This is expected from Fido search, but
But I'm getting this:
I can't understand why |
8e30abf
to
8932bde
Compare
@Cadair, @nabobalis please review this, I have also implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now just comments/suggestions for docstrings, will need to be repeated for each client.
It's good to see these get old PRs brought back. |
I have done all changes suggested by @nabobalis in |
@nabobalis I have done all the suggested changes. |
4ae7145
to
254171b
Compare
@Cadair after a PR related to |
As a note, HelioPy currently has functionality for loading this data: https://docs.heliopy.org/en/stable/reference/data/ace.html |
1b9f89e
to
b7f2c55
Compare
Would you like a reference to this fact in the client somewhere? |
12ba1fb
to
7c51e2d
Compare
@Cadair all tests passed now! |
There was a question on the sunpy call today about the source of the ACE data. The source used here seems to only go back to 2015 despite the fact that the ACE data series goes back much further (into the 1990s). In heliopy, it seems they're using a different data source that presumably covers the entire range of the data: https://github.com/heliopython/heliopy/blob/c55743c00e9c95dc8d317a72730fb0d076560aa7/heliopy/data/ace.py#L4 |
Yea I think the canonical data source would be spdf.gsfc.nasa.giov and since they will probably have to shutdown ftp access at some point in the near future https might be the best option |
I can implement scraper for https as well, since https has superset of data over ftp data. So should this be done after merging this PR in a separate one or should I continue within this PR? |
I think we should swap to the most "official" and complete data source in this PR if that's ok @abhijeetmanhas |
So what's the final decision regarding ACE? I will need some guidance for implementing using above mention source, regarding hdf, cdf, h0, h2, k0, k1,etc various types and what "exactly" the scraper should access? For example here in cdaweb: https://spdf.gsfc.nasa.gov/pub/data/ace/swepam/level_2_cdaweb/ ? All the subfolders or just one of them, if all where I should specify it (filtered through Fido or client query and there is one default) and the format cdf or hdf. |
Closing this now. Thanks everyone for reviewing it, this really helped me to learn a lot! |
In continuation with PR #1806 , I have addressed pep8 issues.
The merge can't be done due to previous versions of
attrs.py
andclient.py
files. I will fix the mergeability issues in future commits of this PR after getting the changes reviewed.