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

EVE ESP timeseries source added #3032

Merged
merged 12 commits into from Apr 10, 2019

Conversation

Projects
None yet
4 participants
@hayesla
Copy link
Contributor

commented Apr 4, 2019

Description

Currently the EVE timeseries supports only level 0 data and cannot handle level1 EVE/ESP data. ESP is a detector which is part of EVE on SDO, providing 4 EUV and 1 soft X-ray irradiance measurements. This follows from #2984

I added a source class names ESPTimeSeries within sunpy/timeseries/sources/eve.py.

Example

from sunpy.net import Fido, attrs as a
from sunpy import timeseries as ts

res = Fido.search(a.Time('2011-02-15 00:00', '2011-02-15 22:00'), a.Instrument('ESP'))
files = Fido.fetch(res)
esp_ts = ts.TimeSeries(files)
esp_ts.peek()

I also added some tests. We should probably include a sample fits for this too.

@sunpy-bot

This comment has been minimized.

Copy link

commented Apr 4, 2019

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

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

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

There is one critical thing wrong with this PR as it stands. It ruins our lovely line count statistics 😝 The test data file you added needs to be marked as binary in git, the easiest way to do this is to put a .fits file extension on the name of the file.

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Also it's 39Mb which is way too big to check into the repo. Can you truncate it or modify it so it's more about ~~100Kb. Thanks!

@Cadair
Copy link
Member

left a comment

A couple of minor tweaks otherwise I think this looks really good!

@@ -0,0 +1 @@
Add timeseries support for EVE/ESP level 1 data in `sunpy.timeseries.sources.eve.py`

This comment has been minimized.

Copy link
@Cadair

Cadair Apr 4, 2019

Member
Suggested change
Add timeseries support for EVE/ESP level 1 data in `sunpy.timeseries.sources.eve.py`
Add timeseries support for EVE/ESP level 1 data in `sunpy.timeseries.sources.eve`
References
----------
* `SDO Mission Homepage <https://sdo.gsfc.nasa.gov/>`_

This comment has been minimized.

Copy link
@Cadair

Cadair Apr 4, 2019

Member

All of these URLs should have __ after rather than _.

Show resolved Hide resolved sunpy/timeseries/sources/eve.py
@ehsteve

ehsteve approved these changes Apr 5, 2019

Copy link
Member

left a comment

I am happy with this and would like to congratulate @hayesla for her first contribution!

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@ehsteve Second one*

@hayesla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

hrmm can't figure out what its failing on - something to do with .peek - anyone any suggestions?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

You created a new figure test with @figure_test and there is no hash to do the comparison to.
E Failed: Hash not present: sunpy.timeseries.tests.test_timeseriesbase.test_esp_peek

So you will need to add the hash to the file of hashes located sunpy/tests/figure_hashes_py36.json. To work out the hash tho involves running the figure test under the figure env using tox -e figure, it should dump out the results into a tmp folder in the root of the sunpy repo.

@hayesla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

You created a new figure test with @figure_test and there is no hash to do the comparison to.
E Failed: Hash not present: sunpy.timeseries.tests.test_timeseriesbase.test_esp_peek

So you will need to add the hash to the file of hashes located sunpy/tests/figure_hashes_py36.json. To work out the hash tho involves running the figure test under the figure env using tox -e figure, it should dump out the results into a tmp folder in the root of the sunpy repo.

So i ran this but cant seem to figure out where the hash is 🤔

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I think it saves out the hashes with the figures in the tmp folder. There should be a bunch of png images and a json file.

@hayesla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

so in my tmp folder there seems to be a sunpydata.sqlite file and a hidden .hypothesis directory with an examples folder full of empty directories - am I missing something or doing something stupid?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Good question, normally when I run the command in my sunpy repository folder, it saves a folder called tmp there (sunpy/tmp/figure_something/. I am not sure if is different on a macbook.

@Cadair
Copy link
Member

left a comment

I am going to put a request changes review on this, so ensure it dosen't accidentally get merged without cleaning up the history!

@hayesla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

oh wait - ran it for a third time and it now is populating my tmp directory 🙉

@hayesla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

hmm there doesn't seem to be a baseline image for comparison?

maybe why the hash doesn't match?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Yeah, I'm not sure. I think we can ignore it and if it fails on merge, we can update it separately.

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

There wont be a comparison image because it hasn't been added to the image repo: https://github.com/sunpy/sunpy-figure-tests which we should do after merging (and also automate!!)

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

We could just squash merge it?

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Yeah, but there is no way to set that on a PR ahead of time, so I just want to make sure we think about it before pressing the button.

@@ -29,6 +29,7 @@ commands = sphinx-build docs docs/_build/html -W -b html

# This env requires tox-conda.
[testenv:figure]
changedir = {toxinidir}

This comment has been minimized.

Copy link
@Cadair

Cadair Apr 8, 2019

Member

@nabobalis I put this here so that when you run the tests locally you get access to the generated files. We don't need the C extensions for this anyway right?

This comment has been minimized.

Copy link
@nabobalis

nabobalis Apr 8, 2019

Contributor

That should be ok but you will need to update the docs, and the circleci path to match.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Apr 8, 2019

Contributor

When I ran it locally, it just made a tmp dir in the sunpy repo folder, it didn't actually use my real tmp volumne.

@Cadair Cadair force-pushed the hayesla:EVE_ESP_timeseries branch from 0b9312e to cbe79ad Apr 8, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Wow this PR has pointed out how spectacularly out of date our reference images are 🤦‍♂

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

We should update them.

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

I opened #3033 as a placeholder for automating it, I will manually update them after this PR lands.

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@hayesla I pushed a commit here which fixed the figure tests, I assume there was something different on your mac setup that tox wasn't controlling correctly. We should probably look into that at somepoint but it's working now 😄

@hayesla

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Great thanks @Cadair !

hayesla and others added some commits Mar 28, 2019

@Cadair Cadair force-pushed the hayesla:EVE_ESP_timeseries branch from cbe79ad to 70de80a Apr 10, 2019

@Cadair

Cadair approved these changes Apr 10, 2019

@ehsteve ehsteve merged commit 9406492 into sunpy:master Apr 10, 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 86.02%)
Details
codecov/project 87.85% (+1.82%) compared to a97d3c6
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190410.1 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
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.