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.

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

(0.93.0) Use with_halos=true default for JLD2OutputWriter #3860

Merged
merged 29 commits into from
Oct 26, 2024
Merged

(0.93.0) Use with_halos=true default for JLD2OutputWriter #3860

merged 29 commits into from
Oct 26, 2024

Conversation

glwagner
Copy link
Member

I keep getting bit by this, so I think its time to make the change. We'll see if there are any tests that assume the default.

@glwagner
Use with_halos=true default for JLD2OutputWriter
c41b892 
I keep getting bit by this, so I think its time to make the change. We'll see if there are any tests that assume the default.
@glwagner
Update jld2_output_writer.jl
9496dbf
navidcy
navidcy approved these changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@navidcy navidcy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm

@navidcy
Copy link
Collaborator

navidcy commented Oct 24, 2024

The Langmuir example was failing because it was loading the arrays with the halos to plot. Switching to using the MakieExt does the job better and cleaner.

@glwagner
Copy link
Member Author

File size in the doctest coming back to slow down development again...

@navidcy
Copy link
Collaborator

navidcy commented Oct 24, 2024

Let’s remove those filesize related doctests!

@glwagner
Copy link
Member Author

Let’s remove those filesize related doctests!

its hard 😭

@navidcy
Copy link
Collaborator

navidcy commented Oct 24, 2024

Let’s remove those filesize related doctests!

its hard 😭

I can do it ;)

@glwagner
Copy link
Member Author

Ah but I think this PR was ready

@glwagner
Copy link
Member Author

Can we do it in a new PR?

@glwagner
Copy link
Member Author

Hmm well I guess its done so lets see if CI passes

@navidcy
Copy link
Collaborator

navidcy commented Oct 24, 2024

oh sure!

@navidcy
Copy link
Collaborator

navidcy commented Oct 24, 2024

Hmm well I guess its done so lets see if CI passes

damn, I reverted and then I saw this...

@glwagner
Copy link
Member Author

feel free to put it back, we have to wait for CI anyways now

@glwagner
Copy link
Member Author

Also I think this is a breaking change

@navidcy
Copy link
Collaborator

navidcy commented Oct 24, 2024

Also I think this is a breaking change

You mean the changing default? Yes, let's bump minor release.

@navidcy navidcy mentioned this pull request Oct 25, 2024
Merged
@navidcy @glwagner
Apply suggestions from code review
a9e6c61 
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
@navidcy
Copy link
Collaborator

navidcy commented Oct 25, 2024

We need CliMA/OrthogonalSphericalShellGrids.jl#44 + a new release of OrthogonalSphericalShellGrids.jl otherwise the cyclical dependency blocks Oceananigans from bumping minor version.

@navidcy navidcy changed the title Use with_halos=true default for JLD2OutputWriter (0.93.0) Use with_halos=true default for JLD2OutputWriter Oct 26, 2024
@navidcy navidcy merged commit b7944c6 into main Oct 26, 2024
46 checks passed
@navidcy navidcy deleted the glw/with-halos branch October 26, 2024 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@navidcy navidcy navidcy approved these changes

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

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