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

(0.95.7) Correctly account for field names in ContinuousBoundaryFunctions #4008

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

(0.95.7) Correctly account for field names in ContinuousBoundaryFunctions #4008

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
193efb7
add masking
simone-silvestri Dec 17, 2024
1c20a5b
also xy fields
simone-silvestri Dec 17, 2024
d1b83b4
improvements
simone-silvestri Dec 18, 2024
ad802d9
add bump
simone-silvestri Dec 18, 2024
7b7639d
Merge branch 'main' into ss/correct-continuous-boundary-functions
simone-silvestri Dec 18, 2024
999b195
bump
simone-silvestri Dec 18, 2024
a861a37
comment
simone-silvestri Dec 18, 2024
0e7dc4a
small correction
simone-silvestri Dec 18, 2024
1d02b9b
couple of corrections
simone-silvestri Dec 18, 2024
d61187e
better comment
simone-silvestri Dec 18, 2024
44fdd9f
correct model_fields
simone-silvestri Dec 18, 2024
1b90451
using field names
simone-silvestri Dec 18, 2024
13d6d57
bugfix
simone-silvestri Dec 18, 2024
fd90c83
validate closure
simone-silvestri Dec 18, 2024
6ae4a6d
whoops not here
simone-silvestri Dec 18, 2024
5a26ecf
not here
simone-silvestri Dec 18, 2024
6f28d5f
add new design
simone-silvestri Dec 18, 2024
b591b45
add tests
simone-silvestri Dec 18, 2024
6ab2279
correction
simone-silvestri Dec 19, 2024
6861ae5
Merge branch 'main' into ss/correct-continuous-boundary-functions
simone-silvestri Dec 19, 2024
b511529
correct
simone-silvestri Dec 22, 2024
39bcdca
some corrections
simone-silvestri Dec 23, 2024
c2da5ee
this should work now
simone-silvestri Dec 23, 2024
19ad053
probably we need to inline
simone-silvestri Dec 23, 2024
f975fc0
bump
simone-silvestri Dec 23, 2024
d9c3177
Merge branch 'main' into ss/correct-continuous-boundary-functions
simone-silvestri Dec 23, 2024
7644409
just do an issue
simone-silvestri Dec 23, 2024
52c248a
Merge branch 'ss/correct-continuous-boundary-functions' of github.com…
simone-silvestri Dec 23, 2024
ddfdab0
Merge branch 'main' into ss/correct-continuous-boundary-functions
navidcy Dec 25, 2024
e4d3867
Merge branch 'main' into ss/correct-continuous-boundary-functions
navidcy Jan 11, 2025
d9fd965
bump patch release
navidcy Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/test_biogeochemistry.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ end
const MB = Union{MinimalDiscreteBiogeochemistry, MinimalContinuousBiogeochemistry}

@inline required_biogeochemical_tracers(::MB) = tuple(:P)
@inline required_biogeochemical_auxiliary_fields(::MB) = tuple(:Iᴾᴬᴿ)
@inline required_biogeochemical_auxiliary_fields(::MB) = ()
Copy link
Member

Choose a reason for hiding this comment

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

We want this for the test?

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that I^{PAR} is repeated in fields. So, indeed, it looks like the bialogical_auxiliary_fields are added to the model's auxiliary fields upon model construction here:

    biogeochemical_fields = merge(auxiliary_fields, biogeochemical_auxiliary_fields(biogeochemistry))
    tracers, auxiliary_fields = validate_biogeochemistry(tracers, biogeochemical_fields, biogeochemistry, grid, clock)

which leads to having repeated I^{PAR} here

@inline fields(model::HydrostaticFreeSurfaceModel) = 
    merge(hydrostatic_fields(model.velocities, model.free_surface, model.tracers), 
          model.auxiliary_fields,
          biogeochemical_auxiliary_fields(model.biogeochemistry))

So I think we have a decision to make
(1) leave the biogeochemical fields only in the model.biogeochemistry
(2) copy them in model.auxiliary_fields but modify the fields function

I guess from previous comments, the intended behavior is option (1), so I will remove the merging of the biogeochemical auxiliaries to the model auxiliaries at model construction.

Copy link
Member

@glwagner glwagner Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

required_biogeochemical_auxiliary_fields are the model.auxiliary_fields. Then there are also biogeochemical_auxiliary_fields(model.biogeochemistry), which are distinct? Notice that model.auxiliary_fields are not accessible from biogeochemical_auxiliary_fields(model.biogeochemistry). So it seems like there are two different kinds of auxiliary fields with bgc.

We can change the design.

@inline biogeochemical_auxiliary_fields(bgc::MB) = (; Iᴾᴬᴿ = bgc.photosynthetic_active_radiation)
@inline biogeochemical_drift_velocity(bgc::MB, ::Val{:P}) = bgc.sinking_velocity

Expand Down
4 changes: 2 additions & 2 deletions test/test_hydrostatic_free_surface_models.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ topos_3d = ((Periodic, Periodic, Bounded),
end

for FreeSurface in (ExplicitFreeSurface, ImplicitFreeSurface, SplitExplicitFreeSurface, Nothing)
@testset "$topo model construction" begin
@info " Testing $free_surface model construction..."
@testset "$FreeSurface model construction" begin
@info " Testing $FreeSurface model construction..."
for arch in archs, FT in float_types
grid = RectilinearGrid(arch, FT, size=(1, 1, 1), extent=(1, 2, 3))
model = HydrostaticFreeSurfaceModel(; grid, free_surface=FreeSurface())
Expand Down
Loading