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 typo in VIIRS geoloc definition #128

Merged
merged 1 commit into from
May 31, 2023

Conversation

frdcms
Copy link
Contributor

@frdcms frdcms commented May 30, 2023

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

@codecov
Copy link

codecov bot commented May 30, 2023

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%) ⬆️

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 djhoese requested review from mraspaud and adybbroe May 30, 2023 14:59
@djhoese
Copy link
Member

djhoese commented May 30, 2023

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.

@djhoese djhoese added the bug label May 30, 2023
@frdcms
Copy link
Contributor Author

frdcms commented May 30, 2023

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 87f0918.

Probably @adybbroe can tell more.

Hope it can help.

Copy link
Contributor

@adybbroe adybbroe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for spotting it!
Are there no tests for this currently, seems not?

@adybbroe
Copy link
Contributor

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
Copy link
Contributor Author

frdcms commented May 31, 2023

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 djhoese self-assigned this May 31, 2023
@djhoese
Copy link
Member

djhoese commented May 31, 2023

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.

@djhoese djhoese merged commit 9eadfef into pytroll:main May 31, 2023
13 checks passed
@frdcms
Copy link
Contributor Author

frdcms commented Jun 20, 2023

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

@frdcms
Copy link
Contributor Author

frdcms commented Jul 12, 2023

@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
Copy link
Member

djhoese commented Jul 12, 2023

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
Copy link
Contributor Author

frdcms commented Jul 12, 2023

Great!
Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants