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

Fix VIIRS L1B reader sensor not matching VIIRS SDR reader #2083

Merged
merged 3 commits into from Apr 11, 2022

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Apr 8, 2022

The 'viirs_sdr' reader produces a lowercase sensor name. The 'viirs_l1b' reader produced whatever case was in the file which for the data I have is uppercase. This is inconsistent and doesn't follow the general standard practice in satpy to have lowercase sensor names.

  • Tests added

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #2083 (69d1315) into main (d4c4332) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2083   +/-   ##
=======================================
  Coverage   93.86%   93.87%           
=======================================
  Files         283      283           
  Lines       42379    42380    +1     
=======================================
+ Hits        39781    39783    +2     
+ Misses       2598     2597    -1     
Flag Coverage Δ
behaviourtests 4.72% <0.00%> (-0.01%) ⬇️
unittests 94.43% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
satpy/readers/viirs_l1b.py 95.77% <100.00%> (+0.60%) ⬆️
satpy/tests/reader_tests/test_viirs_l1b.py 97.26% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4c4332...69d1315. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 8, 2022

Coverage Status

Coverage increased (+0.003%) to 94.372% when pulling 69d1315 on djhoese:bugfix-viirs-l1b-sensor into d4c4332 on pytroll:main.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one minor comment.

return str(res.astype(str))
else:
return res
res = str(res.astype(str))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a small test for this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I went to the work of converting the tests to pytest, created a whole subclass of tests that use numpy array versions of strings only to find that none of the other parts of the file handler use this astype logic. That's because this logic at some point was moved to the base utility netcdf file handler. This branch of code was only meant for older versions of the HDF5 library anyway, but is handled by the np2str usage inside the base class. I've removed these two lines.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It's a good thing you took the time to convert to pytest too :)

@mraspaud
Copy link
Member

Feel free to merge when tests pass.

@djhoese djhoese merged commit d0465f4 into pytroll:main Apr 11, 2022
@djhoese djhoese deleted the bugfix-viirs-l1b-sensor branch April 11, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants