pytroll / pyorbital

Orbital and astronomy computations in python
http://pyorbital.readthedocs.org/
GNU General Public License v3.0
220 stars 75 forks source link

Fix typo in VIIRS geoloc definition #128

Closed frdcms closed 1 year ago

frdcms commented 1 year ago

This PR fixes a typo in the code for VIIRS geoloc definition.

codecov[bot] commented 1 year ago

Codecov Report

Merging #128 (aee96c9) into main (78e58ce) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   87.49%   87.51%   +0.02%     
==========================================
  Files          15       15              
  Lines        2207     2211       +4     
==========================================
+ Hits         1931     1935       +4     
  Misses        276      276              
Flag Coverage Δ
unittests 87.51% <100.00%> (+0.02%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyorbital/geoloc_instrument_definitions.py 71.32% <100.00%> (ø)
pyorbital/tests/test_geoloc.py 100.00% <100.00%> (ø)
djhoese commented 1 year ago

I'm unfamiliar with this part of the code, but could someone tell me where/when this function is used? How was this not caught before? scan_indices defaults to a slice object so this never should have worked in the default case. And if we assume chn_pixels is an integer then the astype shouldn't be needed anyway.

Thanks @frdcms for bringing this to our attention.

frdcms commented 1 year ago

Thanks @djhoese for your comment.

I am not sure this function is widely used. We use it but have not seen the bug before as we have not updated our pyorbital installation for a very long time.

The bug has been introduced in https://github.com/pytroll/pyorbital/commit/87f0918be61f9e47f5eb23e657ac85fa9324258b.

Probably @adybbroe can tell more.

Hope it can help.

adybbroe commented 1 year ago

I agree with @djhoese that this is a bit strange it has been sitting there for so long. I do not recall it. And I have mostly been working with the sounder instruments over the last years. And apparently we do not use this function in our real-time production, otherwise we should have seen it. So go ahead, please.

frdcms commented 1 year ago

Thanks @adybbroe The tests in pytorbital.tests.test_geoloc.TestGeolocDefs.test_viirs all gave the argument scan_indices to geoloc_instrument_definitions.viirs so the bug was not highlighted. Just added another test in order to use the default value for scan_indices in geoloc_instrument_definitions.viirs.

djhoese commented 1 year ago

Awesome! Thanks so much. This looks good to me. I'll merge this, but I'll let @adybbroe decide on when it would be a good time to do a release. It's been a long time since I've done one for pyorbital so it may be better for one of the other maintainers to do it.

frdcms commented 1 year ago

Hello @adybbroe, Do you have any idea when the next release will be created?

frdcms commented 1 year ago

@adybbroe @djhoese Sorry to insist but I need to know whether the next release will be created soon or later to deploy this fix in a production environment. Thanks in advance for your answer.

djhoese commented 1 year ago

This should be included in the 1.8.0 release that is on PyPI. The package will get to conda-forge later today hopefully.

frdcms commented 1 year ago

Great! Thank you very much.