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

replaced noaa with the new json urls #4340

Merged
merged 6 commits into from
Jul 11, 2020
Merged

replaced noaa with the new json urls #4340

merged 6 commits into from
Jul 11, 2020

Conversation

nabobalis
Copy link
Contributor

Fixes #4339

@nabobalis nabobalis requested review from a team as code owners July 8, 2020 13:30
@pep8speaks
Copy link

pep8speaks commented Jul 8, 2020

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

Line 131:101: E501 line too long (126 > 100 characters)
Line 153:101: E501 line too long (126 > 100 characters)

Line 46:101: E501 line too long (102 > 100 characters)
Line 53:101: E501 line too long (113 > 100 characters)
Line 69:101: E501 line too long (106 > 100 characters)
Line 239:101: E501 line too long (120 > 100 characters)
Line 246:101: E501 line too long (109 > 100 characters)

Comment last updated at 2020-07-11 16:44:52 UTC

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks 👍 overall. Should be possible to check the file extension and keep backwards compatibility, but raise a deprecation warning when reading the old files.

@nabobalis nabobalis added this to the 2.1 milestone Jul 8, 2020
@nabobalis nabobalis added backport 2.0 net Affects the net submodule timeseries Affects the timeseries submodule labels Jul 8, 2020
@Cadair Cadair self-requested a review July 8, 2020 15:27
@nabobalis
Copy link
Contributor Author

The dev figure tests are failing and I am unable to build the tox env locally.

@nabobalis
Copy link
Contributor Author

I forgot to check the examples. I will fix that.

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks fine. I think we need the changelog to be a) breaking and b) be a very long user facing description of why and what we are changing so people understand why we have broken their shit on a bugfix.

changelog/4340.bugfix.rst Outdated Show resolved Hide resolved
sunpy/timeseries/sources/noaa.py Outdated Show resolved Hide resolved
@AthKouloumvakos
Copy link

Hi @nabobalis and others! I wonder if it would be an interesting feature/extension for the nooa TimeSeries class, to can handle also other json files (with t.s.) from https://services.swpc.noaa.gov/json/. For example https://services.swpc.noaa.gov/json/goes/ (/primary/xrays-1-day.json) contain json timeseries for soft x-rays provided in real time (contrary to the goes.py class where the fits files are available after 1-2 days). I think this may be something interesting for the sunpy user to have. I attach a simple example of how I process and plot this xrays-1-day.json file, similarly it works for for the other xrays-?-day.json files. Something similar could be tailored to the noaa class, right?
GOES_JSON_FLARE.zip

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a blocker to stop this being merged with upstream issues with the figure tests (see #4342)

sunpy/tests/figure_hashes_mpl_dev_ft_261_astropy_dev.json Outdated Show resolved Hide resolved
@nabobalis nabobalis changed the title replaced noaa with the new json urls! replaced noaa with the new json urls Jul 11, 2020
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, should have a timeseries maintainer look over it.

changelog/4340.bugfix.rst Show resolved Hide resolved
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks 👍 to me. solar_cycle_example.py seems to be failing on the doc build though.

@dstansby dstansby merged commit 49279e2 into sunpy:master Jul 11, 2020
@sunpy-backport
Copy link

The backport to 2.0 failed:

Commits ["73bdc4c440d4fe3accbe8b56bc1a0c648e847a40","63132ccf9cd52e0b23230a70b7f5bbe50c56a7e6","30535cf739134f2275e3f9f2bf2d4951df76bd20","eaf02b13fa03ae226480c38b8e44b7c938c16fc9","3413c5511b28d976c58142e8c563b3fa98aa7317","9a12291aea373b157af2996e89e48b8a7fa7f66e"] could not be cherry-picked on top of 2.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 2.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 73bdc4c440d4fe3accbe8b56bc1a0c648e847a40 63132ccf9cd52e0b23230a70b7f5bbe50c56a7e6 30535cf739134f2275e3f9f2bf2d4951df76bd20 eaf02b13fa03ae226480c38b8e44b7c938c16fc9 3413c5511b28d976c58142e8c563b3fa98aa7317 9a12291aea373b157af2996e89e48b8a7fa7f66e
# Create a new branch with these backported commits.
git checkout -b backport-4340-to-2.0
# Push it to GitHub.
git push --set-upstream origin backport-4340-to-2.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport-4340-to-2.0.

@sunpy-backport
Copy link

The backport to 2.0 failed:

Commits ["73bdc4c440d4fe3accbe8b56bc1a0c648e847a40","63132ccf9cd52e0b23230a70b7f5bbe50c56a7e6","30535cf739134f2275e3f9f2bf2d4951df76bd20","eaf02b13fa03ae226480c38b8e44b7c938c16fc9","3413c5511b28d976c58142e8c563b3fa98aa7317","9a12291aea373b157af2996e89e48b8a7fa7f66e"] could not be cherry-picked on top of 2.0

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub.
git fetch
# Create new working tree.
git worktree add .worktrees/backport 2.0
# Navigate to the new directory.
cd .worktrees/backport
# Cherry-pick all the commits of this pull request and resolve the likely conflicts.
git cherry-pick 73bdc4c440d4fe3accbe8b56bc1a0c648e847a40 63132ccf9cd52e0b23230a70b7f5bbe50c56a7e6 30535cf739134f2275e3f9f2bf2d4951df76bd20 eaf02b13fa03ae226480c38b8e44b7c938c16fc9 3413c5511b28d976c58142e8c563b3fa98aa7317 9a12291aea373b157af2996e89e48b8a7fa7f66e
# Create a new branch with these backported commits.
git checkout -b backport-4340-to-2.0
# Push it to GitHub.
git push --set-upstream origin backport-4340-to-2.0
# Go back to the original working tree.
cd ../..
# Delete the working tree.
git worktree remove .worktrees/backport

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport-4340-to-2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Affects the net submodule timeseries Affects the timeseries submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downloading noaa-indices data via. Fido is broken
5 participants