-
Notifications
You must be signed in to change notification settings - Fork 9
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
Interpolated inital conditions #60
Conversation
using ClimaOcean.Bathymetry: regrid_bathymetry | ||
using ClimaOcean.ECCO2: ecco2_field, ecco2_center_mask | ||
using ClimaOcean.VerticalGrids: stretched_vertical_faces, PowerLawStretching | ||
using ClimaOcean.InitialConditions: three_dimensional_regrid!, adjust_tracers! |
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 start exporting all the things we need so we don't have to have these big using
statements at the top?
using GLMakie | ||
using Oceananigans | ||
using Oceananigans: architecture | ||
using Oceananigans.Utils: prettytime |
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.
This is exported I think?
@@ -0,0 +1,119 @@ | |||
using GLMakie | |||
using Oceananigans | |||
using Oceananigans: architecture |
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.
Might be good to figure out how to get rid of this so users don' thave to worry
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.
agreed
This should be ready to merge, I am not sure about the tests failing though, they are not failing locally |
I am not sure why the docs test is failing. @navidcy do you maybe know why this is happening? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 12 14 +2
Lines 634 765 +131
======================================
- Misses 634 765 +131 ☔ View full report in Codecov by Sentry. |
@@ -47,7 +47,7 @@ end | |||
propagate_horizontally!(field, ::Nothing, tmp_field=deepcopy(field); kw...) = field | |||
|
|||
function propagating(field, mask, iter, max_iter) | |||
mask_sum = sum(field; condition=mask) | |||
mask_sum = sum(field; condition=interior(mask)) |
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.
?
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.
just checking a thing, apparently, we cannot use condition::AbstractField
(because there is no arch_array
method for fields). We could do two things: (1) extend the reductions for field conditions or (2) have an arch_array
also for fields. I think the nice thing to do come up with a single function on_architecture
that is valid for arrays, grids and fields alike
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.
agree that is needed. I guess there is an invalid assumption somewhere that arch_array
works for AbstractArray
.
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.
x-ref: CliMA/Oceananigans.jl#3490
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 think this is a good start. We have a bit more work to do to make this feature more general and also work for boundary conditions but that can come in the future.
@glwagner, @simone-silvestri : shall I merge if tests pass ? |
yes for sure |
Initial conditions (from ECCO fields but also possible to have custom fields)
This PR implements a tool to initialize ClimaOcean's simulations from interpolated tracer fields.
The initial fields need to be supplemented by a mask that is 1 on well-defined values and 0 on missing values
The fields need to be massaged to (i) remove unstable layers (ii) remove unphysical tracer stemming coming from misaligned bathymetry (iii) diffuse initial conditions to prevent instability? (not sure yet)
To-do
Edit: there is no need to diffuse the fields initially. The only caveat is that we need to run the initial 1000/2000 time steps with a low time step size (something like 10 seconds does the trick)