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

Cleanup RSR and LUT download functions for clearer usage #154

Merged
merged 10 commits into from
Sep 16, 2022

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Sep 2, 2022

I was updating some code I have that uses the download functions in pyspectral, but started to get frustrated by lack of docstrings or clear keyword arguments. This PR cleans up docstrings and keyword arguments as well as simplifying some of the logic. It includes one deprecation for the aerosol_type versus aerosol_types keyword argument to be more consistent with the check_and_download functions.

  • Tests added
  • Tests passed: Passes pytest pyspectral
  • Passes flake8 pyspectral
  • Fully documented

@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #154 (39e8dab) into main (f654034) will increase coverage by 4.15%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
+ Coverage   83.79%   87.94%   +4.15%     
==========================================
  Files          23       23              
  Lines        2394     2473      +79     
==========================================
+ Hits         2006     2175     +169     
+ Misses        388      298      -90     
Flag Coverage Δ
unittests 87.94% <98.43%> (+4.15%) ⬆️

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

Impacted Files Coverage Δ
pyspectral/rsr_reader.py 77.99% <66.66%> (+5.56%) ⬆️
pyspectral/utils.py 79.35% <95.74%> (+28.07%) ⬆️
pyspectral/rayleigh.py 95.48% <100.00%> (+4.62%) ⬆️
pyspectral/tests/test_rayleigh.py 100.00% <100.00%> (ø)
pyspectral/tests/test_rsr_reader.py 100.00% <100.00%> (ø)
pyspectral/tests/test_utils.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -283,8 +296,7 @@ def download_rsr(**kwargs):

config = get_config()
local_rsr_dir = config.get('rsr_dir')
dest_dir = kwargs.get('dest_dir', local_rsr_dir)
dry_run = kwargs.get('dry_run', False)
dest_dir = dest_dir or local_rsr_dir
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this tested?

else:
aerosol_types = HTTPS_RAYLEIGH_LUTS.keys()

aerosol_types = _get_aerosol_types(aerosol_types, aerosol_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this part tested?

Copy link
Collaborator

@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.

Nice. Thanks a lot for this clean up. Looks good, I only have one minor comment and a wish for a bit more test-coverage if you agree and find it feasible?

Copy link
Collaborator

@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.

Really nice with the improved/refactored np2str tests also, thanks.
I wonder if we could use the responses module here?
https://pypi.org/project/responses/
I used it a tiny bit in another project recently, after suggestion by @mraspaud
I don't want to add to your long To-Do-List, so if you have done some system-testing verifying that the download functions still work the same, we could also leave it here and add an issue to fix better test coverage of the download functionality?

@djhoese
Copy link
Member Author

djhoese commented Sep 7, 2022

@adybbroe I think I'll add more involved tests still. So far what I added last night was just a placeholder. I'll look at the responses package. If you're OK with it being added then I'm ok with that. I've never used it and I know I did some annoying mocking stuff in Satpy to get something like that working so I'm glad a third-party solution exists.

@adybbroe
Copy link
Collaborator

adybbroe commented Sep 8, 2022

@adybbroe I think I'll add more involved tests still. So far what I added last night was just a placeholder. I'll look at the responses package. If you're OK with it being added then I'm ok with that. I've never used it and I know I did some annoying mocking stuff in Satpy to get something like that working so I'm glad a third-party solution exists.
All good, and thanks @djhoese I am absolutely fine adding another 3rd party package to the dependencies. It will go in the test_requires (or what it is called) list of course.

@djhoese
Copy link
Member Author

djhoese commented Sep 8, 2022

@adybbroe Ok, I think this is ready for re-review. We'll have to wait for the tests to know if code coverage was increased a lot but I'm guessing it is based on my local tests.

Copy link
Collaborator

@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 the improved testing!
Only two small suggestions.
And sorry it took me sol long to get back to you!

@adybbroe adybbroe merged commit e2744cd into pytroll:main Sep 16, 2022
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.

2 participants