Skip to content

(0.96.0) Rename output writers to JLD2Writer and NetCDFWriter #4210

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Mar 18, 2025
Merged

(0.96.0) Rename output writers to JLD2Writer and NetCDFWriter #4210

merged 18 commits into from
Mar 18, 2025

Conversation

tomchor
Copy link
Collaborator

@tomchor tomchor commented Mar 11, 2025

Let's make these names less verbose. I'm starting with the JLD2Writer first and will work on the NetCDFWriter after
#4046 is merged.

@tomchor tomchor changed the title Rename output writers to JLD2Writer and NetCDFWriter (0.96.0) Rename output writers to JLD2Writer and NetCDFWriter Mar 11, 2025
@tomchor tomchor mentioned this pull request Mar 11, 2025
Merged
5 tasks
@glwagner
Copy link
Member

Does it make sense to get #4046 in first and then change the names?

@tomchor
Copy link
Collaborator Author

tomchor commented Mar 11, 2025

Does it make sense to get #4046 in first and then change the names?

Yeah, sorry that's what I meant. I'm started changing the names for JLD2 now, before #4046 is merged, and after that's done, I'm gonna merge main into this PR, change the NetCDFWriter names, and merge. To clarify: I'm proposing doing everything in this PR. Sorry that wasn't clear!

@glwagner
Copy link
Member

Does it make sense to get #4046 in first and then change the names?

Yeah, sorry that's what I meant. I'm started changing the names for JLD2 now, before #4046 is merged, and after that's done, I'm gonna merge main into this PR, change the NetCDFWriter names, and merge. To clarify: I'm proposing doing everything in this PR. Sorry that wasn't clear!

Okay, got it!

@glwagner
Copy link
Member

I am also wondering if we should move the NetCDFWriter to an extension as part of this breaking change. This will mean that users have to write

using NCDatasets

to use NetCDFWriter. The advantage is that we won't have to modify the source code to run on systems that don't support NetCDF, which has happened a few times in the past and might happen more and more as we try to run on more unusual systems.

@tomchor tomchor marked this pull request as ready for review March 17, 2025 23:43
@tomchor
Copy link
Collaborator Author

tomchor commented Mar 17, 2025

I am also wondering if we should move the NetCDFWriter to an extension as part of this breaking change. This will mean that users have to write

using NCDatasets

to use NetCDFWriter. The advantage is that we won't have to modify the source code to run on systems that don't support NetCDF, which has happened a few times in the past and might happen more and more as we try to run on more unusual systems.

I think this makes sense.

@tomchor
Copy link
Collaborator Author

tomchor commented Mar 18, 2025

There is a file called multi_region_output_writers.jl in the MultiRegion module that I'm unsure whether or not to rename. For now I kept it the same since it connects to the OutputWriters module, where NetCDFWriter and JLD2Writer live.

@tomchor tomchor mentioned this pull request Mar 18, 2025
Merged
@glwagner
Copy link
Member

There is a file called multi_region_output_writers.jl in the MultiRegion module that I'm unsure whether or not to rename. For now I kept it the same since it connects to the OutputWriters module, where NetCDFWriter and JLD2Writer live.

We can probably delete this / discontinue support for multi region output writing.

@glwagner
Copy link
Member

If this PR gets hung up, I think it will be ok to tag a couple minor releases, eg up to 0.97 once this is ready.

@glwagner glwagner merged commit d9b3b14 into main Mar 18, 2025
55 checks passed
tomchor referenced this pull request Mar 19, 2025
@tomchor @glwagner
(0.96.0) Move NetCDFWriter to an extension (#4239)
aba99f7 
* start with files that don't require re-indentation

* re-format /examples

* re-format src

* re-format /test

* re-format some validation scripts

* more validation scripts

* more validation scripts

* rest of validation scripts

* fix last validation files

* re-format docs and examples

* reformat src

* reformat test

* reformat validation

* move netcdf_output_writer.jl to extension

* rename files

* move struct definition to OutputWriters

* using much more stuff

* KernelFunctinoOperation is needed

* rename test_netcdf and test_jld2 files

* include correct names

* add NCDatasets to targets

* Update OceananigansNCDatasetsExt.jl

* fix bug in JLD2_writer.jl

* fix examples

* add NCDatasets to validation scripts

* remove blank line

* Use warning instead of error

* Add output_averaging_schedule

* Import more

* using show_array_type

* fix docs/src/model_setup

* Fix lagrangian particle example

---------

Co-authored-by: Gregory L. Wagner <[email protected]>
Co-authored-by: Gregory Wagner <[email protected]>
@navidcy navidcy mentioned this pull request Mar 19, 2025
Merged
@tomchor tomchor mentioned this pull request Mar 25, 2025
Closed
@giordano giordano deleted the tc/netcdfwriter branch April 5, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glwagner glwagner glwagner approved these changes

@ali-ramadhan ali-ramadhan Awaiting requested review from ali-ramadhan

Assignees
No one assigned
Labels
output 💾
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomchor @glwagner @navidcy