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

Feature/backend kwargs json to netcdf #288

Merged
merged 9 commits into from
Feb 28, 2022
Merged

Feature/backend kwargs json to netcdf #288

merged 9 commits into from
Feb 28, 2022

Conversation

EddyCMWF
Copy link
Contributor

Allow backend kwargs to be provided in the to_netcdf executable, either via a json format string, or a path to a json file.
Fixes #286

@EddyCMWF
Copy link
Contributor Author

Hi @alexamici , can you review this PR which fixes issue #286

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2022

Codecov Report

Merging #288 (0abd67d) into master (8e170ec) will decrease coverage by 0.28%.
The diff coverage is 53.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   95.30%   95.01%   -0.29%     
==========================================
  Files          26       26              
  Lines        1938     1947       +9     
  Branches      229      230       +1     
==========================================
+ Hits         1847     1850       +3     
- Misses         61       66       +5     
- Partials       30       31       +1     
Impacted Files Coverage Δ
cfgrib/__main__.py 83.05% <53.33%> (-8.95%) ⬇️

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 8e170ec...0abd67d. Read the comment docs.

@alexamici
Copy link
Contributor

alexamici commented Feb 28, 2022

@EddyCMWF very nice feature!

I have a few style comments:

  • you need to format the code with black you may run the make command to fix code style
  • why do you move imports inside the code? this is a but confusing and both modules are non-optional

@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Feb 28, 2022

My thinking was to improve performance by only importing modules if they are used, but happy to put the imports back to the top of the function.

(Side note, black made my code non-PEP8 compliant _O_/)

@alexamici
Copy link
Contributor

@EddyCMWF

My thinking was to improve performance by only importing modules if they are used, but happy to put the imports back to the top of the function.

My rule is: "readability over performance, unless there is a benchmark" 😄

@EddyCMWF
Copy link
Contributor Author

EddyCMWF commented Feb 28, 2022

@alexamici , sorry to change after your approval, I realised this should have a test in place, so have just added one

@iainrussell
Copy link
Member

Hi @EddyCMWF, thanks for adding the tests! I'm happy to merge this into master if you're finished now.

@EddyCMWF
Copy link
Contributor Author

Hi @iainrussell , yes I am finished now

@iainrussell iainrussell merged commit df30fa2 into ecmwf:master Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add backend_kwargs to cfgrib to_netcdf
4 participants