adding tests for iris tools #4

abit2 · 2017-03-02T14:51:27Z

adding tests for iris tools and corrected the convert_photons_to_DN and DN_to_photons function in IRISRaster class

abit2 · 2017-03-02T14:51:47Z

DanRyanIrish · 2017-03-02T15:16:26Z

Thanks so much @abit2 for this PR and contributing to irispy. I'm going through it now.

DanRyanIrish · 2017-03-02T15:19:51Z

irispy/iris_tools.py

-DETECTOR_GAIN = {"NUV": 18., "FUV": 6., "SJI": 18.}
-DETECTOR_YIELD = {"NUV": 1., "FUV": 1.5, "SJI": 1.}
+DETECTOR_GAIN = {"NUV": 18., "FUV1": 6., "FUV2": 6., "SJI": 18.}
+DETECTOR_YIELD = {"NUV": 1., "FUV1": 1.5, "FUV2": 1.5, "SJI": 1.}


The reason there is only one an FUV key in this dictionary, rather than an FUV1 and FUV2 is that it reflects the metadata supplied by the IRIS instrument team and other aspects of the metadata may only be labelled FUV. Not everything is labelled FUV 1 or FUV2. So I think this should stay FUV for the time being. When determining what detector type to use, when can do something like if "FUV" in detector_type. This will give us the right mapping whether detector_type is "FUV", "FUV1" or "FUV2".

DanRyanIrish · 2017-03-02T15:20:33Z

irispy/iris_tools.py


    Returns
    -------
    data_photons: array-like
        IRIS data in units of photon counts.

    """
+    # if(data.isnan):
+    #     print("HELLO")


Delete these lines

DanRyanIrish

Hi @abit2. Here are my comments on your changes to iris_tools.py and spectrograph.py. I'll look at test_iris_tools.py next.

DanRyanIrish · 2017-03-02T15:21:58Z

irispy/iris_tools.py

 READOUT_NOISE = {"NUV": {"value": 1.2, "unit": "DN"}, "FUV": {"value": 3.1, "unit": "DN"},
                 "SJI": {"value": 1.2, "unit": "DN"}}

-
-def convert_DN_to_photons(data, detector_type):
+def convert_DN_to_photons(data, detector_type='NUV'):


I definitely don't think there should be a default setting for detector type here. The user should have to explicitly give the detector type. There is no reason why NUV is more likely to be wanted by the user than FUV or SJI and could cause the user to get incorrect answers without realising.

DanRyanIrish · 2017-03-02T15:22:47Z

irispy/iris_tools.py

@@ -35,17 +34,20 @@ def convert_DN_to_photons(data, detector_type):
    detector_type: `str`
        Detector/CCD upon which data was recorded.
        Can take values 'NUV', 'FUV' or 'SJI'.
+    default_detector_type : `NUV`


For same reason as above, delete this line.

DanRyanIrish · 2017-03-02T15:23:18Z

irispy/iris_tools.py

    return DETECTOR_GAIN[detector_type]/DETECTOR_YIELD[detector_type]*data


-def convert_photons_to_DN(data, detector_type):
+def convert_photons_to_DN(data, detector_type='NUV'):


Remove default value for detector_type for same reason discussed above.

DanRyanIrish · 2017-03-02T15:41:56Z

irispy/spectrograph.py

@@ -247,19 +247,19 @@ def convert_DN_to_photons(self, spectral_window):
        # Check that DataArray is in units of DN.
        if "DN" not in self.data[spectral_window].attrs["units"]["intensity"]:
            raise ValueError("Intensity units of DataArray are not DN.")
-        self.data[spectral_window].data = iris_tools.convert_DN_to_photons(spectral_window)
+        self.data[spectral_window].data = iris_tools.convert_DN_to_photons(self.data[spectral_window], self.spectral_windows['name'== spectral_window]['detector type'])


Good spot. The calling of this function is indeed wrong. However, the way you've derived the detector_type however raises a VisibleDeprecationWarning because astropy tables in the future will have to be index with integers rather than booleans. I've added an index to this table so you can replace self.spectral_windows['name'== spectral_window]['detector type'] with self.spectral_windows.loc[spectral_window]['detector type'].

Also, replace self.data[spectral_window] with self.data[spectral_window].data in the function call.

DanRyanIrish · 2017-03-02T15:42:44Z

irispy/spectrograph.py



    def convert_photons_to_DN(self, spectral_window):
        """Converts DataArray from DN to photon counts."""
        # Check that DataArray is in units of DN.
        if "photons" not in self.data[spectral_window].attrs["units"]["intensity"]:
            raise ValueError("Intensity units of DataArray are not DN.")
-        self.data[spectral_window].data = iris_tools.convert_photons_to_DN(spectral_window)
+        self.data[spectral_window].data = iris_tools.convert_photons_to_DN(self.data[spectral_window], self.spectral_windows['name'== spectral_window]['detector type'])


As above, replace self.data[spectral_window] with self.data[spectral_window].data and self.spectral_windows['name'== spectral_window]['detector type'] with self.spectral_windows.loc[spectral_window]['detector type'] in function call.

DanRyanIrish · 2017-03-02T15:47:48Z

irispy/tests/test_iris_tools.py

Delete # from start of this line. That was my fault for putting it in the first place.

DanRyanIrish · 2017-03-02T15:51:23Z

irispy/tests/test_iris_tools.py

+	"""
+	"""
+	# source_DN = [[ 0.563004,  1.132289, -1.343129], [-0.719726,  1.441411,  1.566724]]


What's the commented out line? Can it be deleted? The same question for the other commented out source_xxx = [] lines.

DanRyanIrish · 2017-03-02T15:55:58Z

irispy/iris_tools.py

@@ -55,6 +57,7 @@ def convert_photons_to_DN(data, detector_type):
    detector_type: `str`
        Detector/CCD upon which data was recorded.
        Can take values 'NUV', 'FUV' or 'SJI'.
+    default_detector_type : `NUV`


For same reason as above, delete this line.

DanRyanIrish · 2017-03-02T15:56:04Z

irispy/iris_tools.py

@@ -77,6 +80,7 @@ def calculate_intensity_fractional_uncertainty(data, data_unit, detector_type):
    detector_type: `str`
        Detector/CCD upon which data was recorded.
        Can take values 'NUV', 'FUV' or 'SJI'.
+    default_detector_type : `NUV`


For same reason as above, delete this line.

DanRyanIrish · 2017-03-02T21:38:17Z

Thanks @abit2. I had a quick look at your PR it looks good. I'll let you know when I get a chance to make some more comments.

abit2 · 2017-03-03T09:47:39Z

irispy/iris_tools.py

@@ -85,13 +90,17 @@ def calculate_intensity_fractional_uncertainty(data, data_unit, detector_type):
        Same shape as data.

    """
+    if 'FUV' in detector_type:
+        detector_type = 'FUV'
+
    photons_per_dn = DETECTOR_GAIN[detector_type]/DETECTOR_YIELD[detector_type]
    if data_unit == "DN":
        intensity_ph = photons_per_dn*convert_DN_to_photons(data, detector_type)


@DanRyanIrish is this line correct should it be multiplied by photons_per_dn after the data has been converted to photons since in data_unit == 'photons' intensity_ph=data, can you please take a look?

…ter functions

…_response function

DanRyanIrish · 2017-03-03T18:44:23Z

Hi @abit2. This looks good! Could you add one more function to test_iris_tools.py to test that calculate_intensity_fractional_uncertainty() raises a ValueError when the wrong units are passed in. You should be able to do it simply with:

with pytest.raise(ValueError):
    calculate_intensity_fractional_uncertainty(data, wrong_units, detector_type)

DanRyanIrish · 2017-03-03T19:05:35Z

irispy/iris_tools.py

@@ -246,7 +246,7 @@ def get_iris_response(pre_launch=False, response_file=None, response_version=Non


 @custom_model
-def _gaussian1d_on_linear_bg(x, amplitude=None, mean=None, standard_deviation=None,
+def _gaussian1d_on_linear_bg(x=None, amplitude=None, mean=None, standard_deviation=None,


I believe for @custom_model, x should be a positional argument, not a kwarg. So the original version is actually correct.

@DanRyanIrish this happened when I tried this function

This function should not be called directly. It's only used as part of the fitting routine in _calculate_orbital_wavelength_variation(). So you don't need to test this line, if that's what you're trying to do.

…en data_type is DN

DanRyanIrish · 2017-03-03T19:37:03Z

Great stuff. If this passes the Travis test, we should be ready to merge it!

abit2 · 2017-03-03T19:38:10Z

@DanRyanIrish any suggestions on what I should work next

PR#14 changes

DanRyanIrish reviewed Mar 2, 2017

View reviewed changes

DanRyanIrish requested changes Mar 2, 2017

View reviewed changes

DanRyanIrish reviewed Mar 2, 2017

View reviewed changes

DanRyanIrish requested changes Mar 2, 2017

View reviewed changes

abit2 commented Mar 3, 2017

View reviewed changes

abit2 added 3 commits March 3, 2017 23:45

adding tests for iris tools

9941a6e

changes to spectrograph and iris_tools for FUV1/FUV2 and .loc IRISRas…

f26a452

…ter functions

added more test and a exception hadling for response_file in get_iris…

5f81d14

…_response function

abit2 force-pushed the test_iris_tools branch from 1c0a68d to 5f81d14 Compare March 3, 2017 18:17

added more tests for iris_tools

4e199ce

DanRyanIrish reviewed Mar 3, 2017

View reviewed changes

abit2 added 2 commits March 4, 2017 00:42

correcting the calculate_intensity_fractional_uncertainty function wh…

37281ae

…en data_type is DN

removing earlier change

bb1f363

DanRyanIrish approved these changes Mar 6, 2017

View reviewed changes

DanRyanIrish merged commit c9e5377 into sunpy:master Mar 6, 2017

ehsteve pushed a commit that referenced this pull request May 12, 2017

Merge pull request #4 from shelbej/sji-dev

095cc89

PR#14 changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding tests for iris tools #4

adding tests for iris tools #4

abit2 commented Mar 2, 2017

abit2 commented Mar 2, 2017

DanRyanIrish commented Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish left a comment

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish Mar 2, 2017

DanRyanIrish commented Mar 2, 2017

abit2 Mar 3, 2017 •

edited

Loading

DanRyanIrish commented Mar 3, 2017

DanRyanIrish Mar 3, 2017

abit2 Mar 3, 2017

abit2 Mar 3, 2017

DanRyanIrish Mar 3, 2017

DanRyanIrish commented Mar 3, 2017

abit2 commented Mar 3, 2017

adding tests for iris tools #4

adding tests for iris tools #4

Conversation

abit2 commented Mar 2, 2017

abit2 commented Mar 2, 2017

DanRyanIrish commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish left a 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 Mar 2, 2017

abit2 Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

DanRyanIrish commented Mar 3, 2017

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 Mar 3, 2017

abit2 commented Mar 3, 2017

abit2 Mar 3, 2017 •

edited

Loading