Refactor the rsr-reader and add more test coverage #124

adybbroe · 2020-11-12T07:39:20Z

Signed-off-by: Adam.Dybbroe a000680@c21856.ad.smhi.se

Closes Pyspectral incompatible with h5py=3.1.0 #122
Tests added
Tests passed: Passes pytest pyspectral
Passes flake8 pyspectral
Fully documented

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

pyspectral/tests/test_rsr_reader.py

# Conflicts: # pyspectral/rsr_reader.py

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

pyspectral/rsr_reader.py

coveralls · 2020-11-12T08:03:06Z

Coverage increased (+3.4%) to 76.302% when pulling fafcfc2 on adybbroe:fix4-h5py-3.1 into 18f3472 on pytroll:master.

coveralls · 2020-11-12T08:03:07Z

Coverage increased (+0.7%) to 73.587% when pulling e7f9544 on adybbroe:fix4-h5py-3.1 into 18f3472 on pytroll:master.

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

pyspectral/tests/test_rsr_reader.py

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

stickler-ci · 2020-11-12T09:39:53Z

pyspectral/tests/test_rsr_reader.py

+        check_instrument.return_value = None
+
+        with pytest.raises(AttributeError) as exec_info:
+            dummy = RelativeSpectralResponse('MyPlatform')


F841 local variable 'dummy' is assigned to but never used

Replacing dummy with _ will make Stickler happy.

This is still an issue. If you don't use it, assign to _ or don't assign it at all.

mraspaud

LGTM, I just would like to see one function refactored :)

mraspaud · 2020-11-12T09:42:48Z

pyspectral/rsr_reader.py

+    def get_relative_spectral_responses(self, h5f):
+        """Read the rsr data and add to the object."""
+        no_detectors_message = False
+        for bandname in self.band_names:
+            self.rsr[bandname] = {}
+            try:
+                num_of_det = h5f[bandname].attrs['number_of_detectors']
+            except KeyError:
+                if not no_detectors_message:
+                    LOG.debug("No detectors found - assume only one...")
+                num_of_det = 1
+                no_detectors_message = True
+
+            for i in range(1, num_of_det + 1):
+                dname = 'det-{0:d}'.format(i)
+                self.rsr[bandname][dname] = {}
+                try:
+                    resp = h5f[bandname][dname]['response'][:]
+                except KeyError:
+                    resp = h5f[bandname]['response'][:]
+
+                self.rsr[bandname][dname]['response'] = resp
+
+                try:
+                    wvl = (h5f[bandname][dname]['wavelength'][:] *
+                           h5f[bandname][dname][
+                               'wavelength'].attrs['scale'])
+                except KeyError:
+                    wvl = (h5f[bandname]['wavelength'][:] *
+                           h5f[bandname]['wavelength'].attrs['scale'])
+
+                # The wavelength is given in micro meters!
+                self.rsr[bandname][dname]['wavelength'] = wvl * 1e6
+
+                try:
+                    central_wvl = h5f[bandname][
+                        dname].attrs['central_wavelength']
+                except KeyError:
+                    central_wvl = h5f[bandname].attrs['central_wavelength']
+
+                self.rsr[bandname][dname][
+                    'central_wavelength'] = central_wvl
+


This function should be refactored, it is too long and cryptic

I agree! I will do!

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

ghost · 2020-11-12T15:05:31Z

Congratulations 🎉. DeepCode analyzed your code in 2.677 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

pyspectral/rsr_reader.py

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

adybbroe · 2020-11-13T15:10:59Z

Ok to merge @pnuu ?
I don't see how I should improve the deepcode issue. And I think the stickler issues should be gone...

djhoese · 2020-11-13T15:21:02Z

pyspectral/tests/test_rsr_reader.py

+
+        test_rsr.rsr['M1'] = {'det-1': {}}
+        test_rsr.set_band_central_wavelength_per_detector(h5f, 'M1', 'det-1')
+        self.assertTrue('central_wavelength' in test_rsr.rsr['M1']['det-1'])


You could use assertIn here to make deepcode happier: https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertIn

Right?

Oh, yes, right! thx!

pnuu

Just couple of comments. Feel free to merge.

pnuu · 2020-11-13T15:18:22Z

pyspectral/tests/test_rsr_reader.py

+        check_instrument.return_value = None
+
+        with pytest.raises(AttributeError) as exec_info:
+            dummy = RelativeSpectralResponse('MyPlatform')


Replacing dummy with _ will make Stickler happy.

pnuu · 2020-11-13T15:20:25Z

pyspectral/tests/test_utils.py

 import unittest
 import numpy as np

+from pyspectral import utils
+from pyspectral.utils import np2str, bytes2string


I think the functions that are tested should be imported inside the test function/method. That way they can be mocked in other tests, if it becomes necessary.

Okay, I thought the mocking was independent of the imports actually!?
I always have all imports at the top.

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Refactor the rsr-reader and add more test coverage

87a3729

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

adybbroe requested review from mraspaud and pnuu November 12, 2020 07:39

adybbroe self-assigned this Nov 12, 2020

stickler-ci reviewed Nov 12, 2020

View reviewed changes

pyspectral/tests/test_rsr_reader.py Outdated Show resolved Hide resolved

pyspectral/tests/test_rsr_reader.py Outdated Show resolved Hide resolved

pyspectral/tests/test_rsr_reader.py Outdated Show resolved Hide resolved

Adam.Dybbroe added 2 commits November 12, 2020 08:53

Merge branch 'master' into fix4-h5py-3.1

80b0331

# Conflicts: # pyspectral/rsr_reader.py

Remove debug stuff

e7f9544

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

stickler-ci reviewed Nov 12, 2020

View reviewed changes

pyspectral/rsr_reader.py Outdated Show resolved Hide resolved

pyspectral/rsr_reader.py Outdated Show resolved Hide resolved

Add more test coverage

d47b014

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

stickler-ci reviewed Nov 12, 2020

View reviewed changes

pyspectral/tests/test_rsr_reader.py Outdated Show resolved Hide resolved

adybbroe marked this pull request as ready for review November 12, 2020 09:37

Fix for flake8

798e3b7

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

stickler-ci reviewed Nov 12, 2020

View reviewed changes

adybbroe added bug enhancement labels Nov 12, 2020

mraspaud reviewed Nov 12, 2020

View reviewed changes

Refactor the part setting the actual RSR data

a721d33

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Adam.Dybbroe added 2 commits November 12, 2020 17:33

Fix for travis-ci

c1e1852

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Bugfix: Wrong use of np2str

0527514

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

stickler-ci reviewed Nov 12, 2020

View reviewed changes

pyspectral/rsr_reader.py Outdated Show resolved Hide resolved

Adam.Dybbroe added 2 commits November 12, 2020 17:42

Remove old commented out code

74fa101

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Refactor

13df3ec

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

djhoese reviewed Nov 13, 2020

View reviewed changes

pnuu approved these changes Nov 13, 2020

View reviewed changes

Adam.Dybbroe added 2 commits November 13, 2020 16:23

Make stickler happy.

bb2c35d

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

Make deepcode happy, provide more information on assert failures

fafcfc2

Signed-off-by: Adam.Dybbroe <a000680@c21856.ad.smhi.se>

adybbroe merged commit 0f2eedb into pytroll:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the rsr-reader and add more test coverage #124

Refactor the rsr-reader and add more test coverage #124

adybbroe commented Nov 12, 2020 •

edited

coveralls commented Nov 12, 2020 •

edited

coveralls commented Nov 12, 2020

stickler-ci Nov 12, 2020

pnuu Nov 13, 2020

djhoese Nov 13, 2020

mraspaud left a comment

mraspaud Nov 12, 2020

adybbroe Nov 12, 2020

ghost commented Nov 12, 2020 •

edited by ghost

adybbroe commented Nov 13, 2020

djhoese Nov 13, 2020

adybbroe Nov 13, 2020

pnuu left a comment

pnuu Nov 13, 2020

pnuu Nov 13, 2020

adybbroe Nov 13, 2020

Refactor the rsr-reader and add more test coverage #124

Refactor the rsr-reader and add more test coverage #124

Conversation

adybbroe commented Nov 12, 2020 • edited

coveralls commented Nov 12, 2020 • edited

coveralls commented Nov 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 12, 2020 • edited by ghost

👉 View analysis in DeepCode’s Dashboard | Configure the bot

adybbroe commented Nov 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnuu 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

adybbroe commented Nov 12, 2020 •

edited

coveralls commented Nov 12, 2020 •

edited

ghost commented Nov 12, 2020 •

edited by ghost