The Great Purge mk II #2666

Cadair · 2018-06-20T18:08:22Z

This deletes all the deprecated submodules for 1.0

ghost · 2018-06-20T18:08:28Z

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

Cadair · 2018-06-20T18:08:51Z

@pep8speaks quiet

Cadair · 2018-06-20T22:01:54Z

@DanRyanIrish this refactors the goes em code to use timeseries if you want to have a look

Cadair · 2018-06-27T11:16:54Z

note to self: I have merged this into the time branch, do not rebase this branch

nabobalis · 2018-06-27T11:20:18Z

Note to self: MERGE IT NOW, PRESS THE BUTTON.

DanRyanIrish · 2018-07-06T17:15:23Z

sunpy/instr/tests/test_lyra.py

-                                             "CHANNEL2": CHANNELS[1],
-                                             "CHANNEL3": CHANNELS[0],
-                                             "CHANNEL4": CHANNELS[1]})
+def ts():


Can we have a more descriptive function name?

DanRyanIrish · 2018-07-06T17:20:28Z

sunpy/instr/goes.py

-@u.quantity_input(longflux=u.W/u.m/u.m, shortflux=u.W/u.m/u.m)
-def _goes_chianti_tem(longflux, shortflux, satellite=8,
+@u.quantity_input
+def _goes_chianti_tem(longflux: u.W/u.m/u.m, shortflux: u.W/u.m/u.m, satellite=8,


This syntax is new to me. Presumably it means longflux input must be a quantity with equivalent units to u.W/u.m/u.m?

DanRyanIrish · 2018-07-06T17:23:03Z

sunpy/instr/goes.py

@@ -774,13 +772,13 @@ def calculate_radiative_loss_rate(goeslc, force_download=False,

    Returns
    -------
-    lc_new : `~sunpy.lightcurve.LightCurve`
+    lc_new : `~sunpy.timeseries.XRSTimeSeries`


This Should be be ts_new

DanRyanIrish · 2018-07-06T17:25:12Z

sunpy/instr/goes.py

-    2014-01-01 00:00:04.518999  9.187300e-08  0.000004     2.498454e+16     9.530365e+17
-    2014-01-01 00:00:06.564999  9.298800e-08  0.000004     2.528776e+16     9.530365e+17
+    >>> import sunpy.timeseries as ts
+    >>> from sunpy.instr.goes import calculate_temperature_em


This should be >>> from sunpy.instr.goes import calculate_xray_luminosity

DanRyanIrish · 2018-07-06T17:26:44Z

sunpy/instr/goes.py

-                                   data=copy.deepcopy(goeslc.data))
-    lc_new.data["luminosity_xrsa"] = lx_out["shortlum"].to("W").value
-    lc_new.data["luminosity_xrsb"] = lx_out["longlum"].to("W").value
+    lc_new = timeseries.XRSTimeSeries(meta=copy.deepcopy(goests.meta),


lc_new should be ts_new here.

DanRyanIrish · 2018-07-06T17:26:49Z

sunpy/instr/goes.py

+    lc_new = timeseries.XRSTimeSeries(meta=copy.deepcopy(goests.meta),
+                                      data=copy.deepcopy(goests.data),
+                                      units=copy.deepcopy(goests.units))
+    lc_new = lc_new.add_column("luminosity_xrsa", lx_out["shortlum"].to("W"))


lc_new should be ts_new here.

DanRyanIrish · 2018-07-06T17:26:53Z

sunpy/instr/goes.py

+                                      data=copy.deepcopy(goests.data),
+                                      units=copy.deepcopy(goests.units))
+    lc_new = lc_new.add_column("luminosity_xrsa", lx_out["shortlum"].to("W"))
+    lc_new = lc_new.add_column("luminosity_xrsb", lx_out["longlum"].to("W"))


lc_new should be ts_new here.

DanRyanIrish · 2018-07-06T17:26:59Z

sunpy/instr/goes.py

+                                      data=copy.deepcopy(goests.data),
+                                      units=copy.deepcopy(goests.units))
+    lc_new = lc_new.add_column("luminosity_xrsa", lx_out["shortlum"].to("W"))
+    lc_new = lc_new.add_column("luminosity_xrsb", lx_out["longlum"].to("W"))

    return lc_new


lc_new should be ts_new here.

DanRyanIrish · 2018-07-06T17:31:21Z

sunpy/instr/tests/test_goes.py


    # Test case 2: GOESLightCurve object with flux and temperature
    # data, but no EM data.
    goes_test = goes.calculate_radiative_loss_rate(goeslc_no_em)
-    assert_frame_equal(goeslc_test.data, goeslc_expected.data)
+    # we test that the column has been added
+    assert "rad_loss_rate" in goes_test.columns


Why do we only test the column is added here and not that the values are correct?

done using pytest-arraydiff

DanRyanIrish · 2018-07-06T17:35:43Z

sunpy/instr/tests/test_lyra.py

    return lyrats


 @pytest.mark.remote_data
-@pytest.mark.parametrize('dtype', ['ts', 'lc'])
-def test_remove_lytaf_events_from_timeseries(dtype):
+def test_remove_lytaf_events_from_timeseries(ts):
    """Test if artefacts are correctly removed from a TimeSeries.


This docstring probably needs updating so it's clear what the input is. The mention of the LYRALightCurve can probably go too.

DanRyanIrish · 2018-07-06T17:39:40Z

Now that all the lightcurve sources have been removed, can you remind me how TimeSeries handles different sources? It's a factory-like setup like Map which detects the source from the input file?

vn-ki · 2018-07-06T17:43:14Z

.circleci/early_exit.sh

+#!/bin/bash
+
+commitmessage=$(git log --pretty=%B -n 1)
+if [[ $commitmessage = *"[ci skip]"* ]]; then


Travis supports both [skip ci] and [ci skip]. Personally I prefer, [skip ci]. Shouldn't we support both?

vn-ki · 2018-07-06T17:49:52Z

sunpy/instr/goes.py

@@ -754,8 +752,8 @@ def calculate_radiative_loss_rate(goeslc, force_download=False,

    Parameters
    ----------
-    goeslc : `~sunpy.lightcurve.LightCurve`
-        LightCurve object containing GOES data.  The units of these
+    goeslc : `~sunpy.timeseries.XRSTimeSeries`


You missed ts here.

vn-ki · 2018-07-06T17:58:52Z

sunpy/instr/tests/test_goes.py

@@ -67,17 +67,17 @@ def test_goes_event_list():
 def test_calculate_temperature_em():
    # Create GOESLightcurve object, then create new one with


Comment and the rest of the function needs updating.

Cadair · 2018-07-18T15:03:10Z

@DanRyanIrish yes if it can be autodetected, i.e. its not a csv file, there is also a format= kwarg for when it cant be auto detected.

sunpy deleted a comment from pep8speaks Jun 20, 2018

Cadair added this to the 1.0 milestone Jun 20, 2018

Cadair force-pushed the removalmk2 branch from 2a2789e to 361dd4d Compare June 20, 2018 18:20

Cadair added the Refactoring Code changes without affecting API (generally) label Jun 20, 2018

Cadair requested review from dpshelio and nabobalis June 20, 2018 19:48

Cadair added the Needs Review Needs reviews before merge label Jun 20, 2018

nabobalis approved these changes Jun 20, 2018

View reviewed changes

Cadair requested a review from DanRyanIrish June 20, 2018 22:01

Cadair force-pushed the removalmk2 branch 2 times, most recently from a854cda to 42aa747 Compare June 22, 2018 13:06

Cadair added 9 commits June 22, 2018 14:12

Remove spectra

6edfe78

Remove wcs

32b8c6b

port instr.goes to using TimeSeries

f2a54e8

Remove lightcurve

2e7199b

other cleanup

866b65e

remove spectra references from the docs

bf54be3

Fix the instr.goes doctests

7b0508d

Add changelog entries [ci skip]

2b35226

Implement a poor mans skip for circle

659f282

Cadair force-pushed the removalmk2 branch from 42aa747 to 659f282 Compare June 22, 2018 13:12

DanRyanIrish reviewed Jul 6, 2018

View reviewed changes

vn-ki reviewed Jul 6, 2018

View reviewed changes

Cadair mentioned this pull request Jul 18, 2018

Migrate to Astropy Time #2691

Merged

2 tasks

Cadair added 2 commits July 18, 2018 15:35

First batch of changes based on review

e2438e7

use pytest arraydiff to compare the values of the goes tests

bc8bbd1

tricksy hobbits

0818cf2

nabobalis approved these changes Jul 19, 2018

View reviewed changes

Cadair merged commit 98cced8 into sunpy:master Jul 19, 2018

Cadair deleted the removalmk2 branch July 19, 2018 10:29

nabobalis added this to Finished in SunPy 1.0 Sep 24, 2018

nabobalis mentioned this pull request Jan 22, 2019

GOES functions need to be updated to handle XRSTimeSeries class. #2628

Closed

dstansby removed the Needs Review Needs reviews before merge label Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Great Purge mk II #2666

The Great Purge mk II #2666

Cadair commented Jun 20, 2018

ghost commented Jun 20, 2018 •

edited by ghost

Cadair commented Jun 20, 2018

Cadair commented Jun 20, 2018

Cadair commented Jun 27, 2018

nabobalis commented Jun 27, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

Cadair Jul 18, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish Jul 6, 2018

Cadair Jul 18, 2018

DanRyanIrish Jul 6, 2018

DanRyanIrish commented Jul 6, 2018

vn-ki Jul 6, 2018

vn-ki Jul 6, 2018

vn-ki Jul 6, 2018

Cadair commented Jul 18, 2018

		@@ -67,17 +67,17 @@ def test_goes_event_list():
		def test_calculate_temperature_em():
		# Create GOESLightcurve object, then create new one with

The Great Purge mk II #2666

The Great Purge mk II #2666

Conversation

Cadair commented Jun 20, 2018

ghost commented Jun 20, 2018 • edited by ghost

Cadair commented Jun 20, 2018

Cadair commented Jun 20, 2018

Cadair commented Jun 27, 2018

nabobalis commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadair commented Jul 18, 2018

ghost commented Jun 20, 2018 •

edited by ghost