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

Update NetCDFOutputWriter docstring and clobber default #556

Merged
merged 6 commits into from
Dec 11, 2019

Conversation

ali-ramadhan
Copy link
Member

@ali-ramadhan ali-ramadhan commented Dec 7, 2019

Docstring did not match the actual constructor. They do now.

@suyashbire1 Do you think clobber should be true or false by default?

Also updated the docstring to make it clear that outputs needs to be a Dict for now.

Resolves #553

@ali-ramadhan ali-ramadhan added bug 🐞 Even a perfect program still has bugs documentation 📜 The sacred scrolls labels Dec 7, 2019
@codecov
Copy link

codecov bot commented Dec 7, 2019

Codecov Report

Merging #556 into master will increase coverage by 3.74%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   67.72%   71.47%   +3.74%     
==========================================
  Files          69       69              
  Lines        1952     2033      +81     
==========================================
+ Hits         1322     1453     +131     
+ Misses        630      580      -50
Impacted Files Coverage Δ
src/OutputWriters/netcdf_output_writer.jl 86.53% <100%> (ø) ⬆️
src/utils.jl 75.56% <0%> (+3.99%) ⬆️
src/Diagnostics/cfl.jl 71.42% <0%> (+4.76%) ⬆️
src/Operators/interpolation_operators.jl 64.28% <0%> (+4.76%) ⬆️
src/Operators/derivative_operators.jl 90.62% <0%> (+32%) ⬆️
...lementations/anisotropic_biharmonic_diffusivity.jl 100% <0%> (+50%) ⬆️
...ure_implementations/leith_enstrophy_diffusivity.jl 98.38% <0%> (+66.12%) ⬆️
src/Operators/laplacian_operators.jl 100% <0%> (+85.71%) ⬆️

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 a511fdd...e187583. Read the comment docs.

@glwagner
Copy link
Member

glwagner commented Dec 9, 2019

clobber=false is probably safer.

@ali-ramadhan ali-ramadhan changed the title Update NetCDFOutputWriter docstring Update NetCDFOutputWriter docstring and clobber default Dec 9, 2019
`Dict` or `NamedTuple`) to a NC file, where `label` is a symbol that labels the output and
`field` is a field from the model (e.g. `model.velocities.u`).
Construct a `NetCDFOutputWriter` that writes `label, field` pairs in `outputs` (which should
be a `Dict`) to a NC file, where `label` is a symbol that labels the output and `field` is
Copy link
Member

Choose a reason for hiding this comment

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

why can't we handle named tuples... ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would be easy to add support for named tuple. I just removed them from the docstring because of #553

We can add support for named tuples in this PR as well.

@glwagner
Copy link
Member

Named tuples are trivial to support; but maybe users don't care. Personally, I much prefer them in the JLD2OutputWriter.

If mode="a" then write_grid will almost always fail. I can't think of a 
reason to append to a grid anyways.
@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Dec 11, 2019

clobber=false is probably safer.

That's what I was thinking too but apparently with NCDatasets.jl the other modes are append ("a") and read-only ("r") so if you're creating files for the first time (the most common use case) then clobber must be used. Otherwise you get NetCDF errors when it tries to open a non-existent file.

Named tuples are trivial to support; but maybe users don't care. Personally, I much prefer them in the JLD2OutputWriter.

I actually wasn't able to get named tuples to work with JLD2 but I opened an issue about that: #562

I don't see the big advantage of a NamedTuple over a Dict in this case so I'm going to keep it simple and stick to named tuples here for now. The original point of this PR was that the documentation was wrong.

NetCDFOutputWriter needs to be refactored a little anyways, and I'd like to add some features to it as well so I'll add support for named tuples then.

@ali-ramadhan ali-ramadhan merged commit d3c0157 into master Dec 11, 2019
@ali-ramadhan ali-ramadhan deleted the ali-ramadhan-patch-2 branch December 11, 2019 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Even a perfect program still has bugs documentation 📜 The sacred scrolls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NetCDFOutputWriter errors when given outputs as named tuple
2 participants