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

Interpolated inital conditions #60

Merged
merged 61 commits into from
Mar 28, 2024
Merged

Interpolated inital conditions #60

merged 61 commits into from
Mar 28, 2024

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Nov 16, 2023

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

  • assess if it is better to inpaint on the initial field or the final field (still allow both options)
  • assess whether we need to diffuse the initial field or not

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)

@simone-silvestri simone-silvestri changed the title Ss/inital conditions Interpolated inital conditions for ClimaOcean Nov 16, 2023
@simone-silvestri simone-silvestri changed the title Interpolated inital conditions for ClimaOcean Interpolated inital conditions Nov 16, 2023
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!
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed

@simone-silvestri simone-silvestri marked this pull request as ready for review November 21, 2023 20:06
@simone-silvestri
Copy link
Collaborator Author

This should be ready to merge, I am not sure about the tests failing though, they are not failing locally

@simone-silvestri
Copy link
Collaborator Author

I am not sure why the docs test is failing. @navidcy do you maybe know why this is happening?

@navidcy
Copy link
Collaborator

navidcy commented Feb 27, 2024

I am not sure why the docs test is failing. @navidcy do you maybe know why this is happening?

Yeap, few docstrings were not included in the docs. I fixed it. I added them in the docs but also added an option so that we only get a warning when that happens; see 3234942

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 0% with 212 lines in your changes are missing coverage. Please review.

Project coverage is 0.00%. Comparing base (1bd1700) to head (6bb9422).

Files Patch % Lines
src/DataWrangling/ECCO2.jl 0.00% 58 Missing ⚠️
src/DataWrangling/inpaint_mask.jl 0.00% 56 Missing ⚠️
src/InitialConditions/diffuse_tracers.jl 0.00% 39 Missing ⚠️
src/Bathymetry.jl 0.00% 34 Missing ⚠️
src/InitialConditions/InitialConditions.jl 0.00% 24 Missing ⚠️
src/VerticalGrids.jl 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

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

Copy link
Member

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@navidcy navidcy requested a review from glwagner March 6, 2024 11:08
Copy link
Member

@glwagner glwagner left a 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.

@navidcy
Copy link
Collaborator

navidcy commented Mar 27, 2024

@glwagner, @simone-silvestri : shall I merge if tests pass ?

@glwagner
Copy link
Member

yes for sure

@glwagner glwagner merged commit de17150 into main Mar 28, 2024
9 checks passed
@navidcy navidcy deleted the ss/inital-conditions branch March 28, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants