-
Notifications
You must be signed in to change notification settings - Fork 191
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
Passes grid
argument to NetCDFOutputWriter
#3576
Merged
Merged
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
48aa15c
Update netcdf_output_writer.jl
tomchor f0143e1
Add grid as property to NetCDFOutputWriter and use it during file ini…
glwagner fdee401
Merge branch 'main' into tc/grid-to-netcdfwriter
glwagner 296ab22
paragraph explaining that netcdf writer also accepts an output grid
iuryt 1ce9908
change wording and add jldoctest
iuryt fa97a72
Update docs/src/model_setup/output_writers.md
iuryt 077fea6
Do a little better with function output for NetCDF output writer
glwagner c61729b
Fixes interpolate! plus some improvements to NetCDFOutputWriter
glwagner 4134b5b
add new example for docs
iuryt ed5ce3e
simplify example
iuryt 373f15a
simplify example in the docstring
iuryt 6f0b9fb
Update docs/src/model_setup/output_writers.md
iuryt 42ccd03
Make grid a kwarg and update language
glwagner 8a7d7c0
Merge branch 'main' into tc/grid-to-netcdfwriter
glwagner 14c94d8
Merge branch 'main' into tc/grid-to-netcdfwriter
tomchor 70e459a
Update src/OutputWriters/netcdf_output_writer.jl
iuryt 9403243
Update docs/src/model_setup/output_writers.md
iuryt 45f0293
Update netcdf_output_writer.jl
iuryt 8c141de
Bugfix plus better error message
glwagner c4357e5
Correctly define_output_variable for LagrangianParticles
glwagner 4a3bd34
Merge branch 'main' into tc/grid-to-netcdfwriter
glwagner aa5e65c
Fix znodes in output writers docs
glwagner ab73354
fix znodes
iuryt a3a2f7e
fix double semicolon
iuryt 4850ddc
14.5 kB
glwagner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that once
NetCDFWriter
hasoutput_grid
passed it can only write outputs on that grid? In other words, you can't have outputs that are the original/full grid and the interpolated/coarse grid in the sameNetCDFWriter
file (for now at least). I feel this can be a source of confusion for users in the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we don't pass
output_grid
. We pass the keywordgrid
, correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We made it a positional argument. But we can change that and make a keyword argument for sure. What do you think?
Also, I agree 100% that making that more explicit is a good idea. The
grid
and outputs must be consistent, otherwise output writing will fail.Also, you often write
NetCDFWriter
. I like that name because it's a little more concise ("output" is obvious). Should we change this andJLD2
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that
NetCDFWriter
is better and that we should also change that for JLD.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, after this is merged we will get on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true. I have a bit of a preference for keyword arguments, but I don't feel strongly about it.
Ah, yeah, sorry. I write that for the exact reason you mentioned, and the fact that
NetCDFOutputWriter
is kinda long lolI often thought abut opening an issue to suggest that change myself, but haven't gotten around to it. So I'm very much in favor of making that change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm ok I agree it should be a kwarg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the docs examples are identical to the doc string examples. Ideally these would be different... there's no point in copy pasting here. It's more efficient to use Documenter to insert the docstring if we decide that we don't have the bandwidth to come up with unique examples.
But I personally think that the docstring and docs should have different examples. The docstring should have short concise examples and the docs should go into more detail.
Anyways, task for the future I guess.