-
Notifications
You must be signed in to change notification settings - Fork 21
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
Generalize TwoBandPhotosyntheticallyActiveRadiation
kernel to allow curvilinear grids
#120
Conversation
Not sure this is the most efficient way todo this, but since `xnode` is defiend differedntly for different grid types this is by far the easiest fix. The alternative would be to define `xnode(i, grid, lx, ly, lz)` for RectilinearGrids. Perhaps that solution would be better.
Not sure this is the most efficient way todo this, but since |
Probably should add some tests on Lat/lon grids |
What do you mean by
? |
xnode(i, j, k, grid, ℓx, ℓy, ℓz) should work for all grids. If the above is not true then we should fix it in Oceananigans. Note that |
src/Light/2band.jl
Outdated
@@ -1,7 +1,7 @@ | |||
@kernel function update_TwoBandPhotosyntheticallyActiveRadiation!(PAR, grid, P, surface_PAR, t, PAR_model) | |||
i, j = @index(Global, NTuple) | |||
|
|||
x, y = xnode(i, grid, Center()), ynode(j, grid, Center()) | |||
x, y = @inbounds xnodes(grid, Center(), Center(), Center())[i], ynodes(grid, Center(), Center(), Center())[j] |
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.
not sure if k=1
is what we want here, but how about:
x, y = @inbounds xnodes(grid, Center(), Center(), Center())[i], ynodes(grid, Center(), Center(), Center())[j] | |
x = xnode(i, j, 1, grid, Center(), Center(), Center()) | |
y = ynode(i, j, 1, grid, Center(), Center(), Center()) |
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.
Note that for general curvilinear grids, the nodes depend on both i, j
. So you probably want to use @navidcy's suggestion.
All current grids (and for the forseeable future) are independent of k
so it's ok to take k=1
. This makes a "thin spherical shell" approximation for the ocean.
Thank you both @navidcy @glwagner . I'm not sure exactly what I've got confused by, I think it was trying to do I was trying to run one of the ClimaOcean examples with a BGC model which initially wasn't working because the light just had |
Note that ClimaOcean might need some updating! |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #120 +/- ##
=======================================
Coverage 61.46% 61.46%
=======================================
Files 26 26
Lines 968 968
=======================================
Hits 595 595
Misses 373 373
☔ View full report in Codecov by Sentry. |
It does work now
I did have to remove some compat entries to install OceanBioME with it but it seems to be running fine with Oceananigans 0.83.0 |
xnode
use to xnodes
TwoBandPhotosyntheticallyActiveRadiation
kernel to allow curvilinear grids
src/Models/Individuals/SLatissima.jl
Outdated
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.
shall we delete commented code? it's unclear why it's there..
#nodes, weights, normfactor = @inbounds get_nearest_nodes(x, y, z, grid, (Center(), Center(), Center()))
#@unroll for (n, weight) in enumerate(weights)
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.
Oh yeah, not sure why that's still there
I've just thought, it's probably more convenient for a user if the surface PAR is in the grid native coordinates, for example, if doing a global simulation data is going to be in lat/lon coordinates. Using |
node(i, j, k, grid, ℓx, ℓy, ℓz) will return a 1-, 2-, or 3- tuple with the native coordinates of the grid. (1-, 2-, or 3-tuple depending if any of the coordinates are Flat). |
Oh yeah I mean to fix this. Although as I've done it now, if it returns anything other than a 3-tuple its going to fail right? Is there a way this can work for Flat dimensions? |
hm... I think what you did won't work... because you seem to assume the |
So I'm not sure what you are trying to do so I can't be very specific. locs = node(i, j, k, grid, Center(), Center(), Center())
value = surface_PAR(loss...) This way you don't unfold the tuple but just pass it to the |
I just checked and it seems to work on flat dimensions always returning a 3-tuple, as long as the locations are all not |
I guess this would work although we currently have |
Hm... But this, e.g., |
Sorry, ignore my suggestion then. Perhaps I just don't know what you are trying to do and I was assuming things. Let's restart: Yes, to get the coordinates of the grid at |
Since we're always giving the locs as Center/Center/Center won't it always use the first method though? I guess it uses the others if you were to e.g. do |
True! My bad... |
should we add a test? |
I've changed the light test to test on different grids and models now, I don't think we need the other tests todo this since the others just test the properties of the bgc model components and their ability to work in different grids etc. should be being tested in the Oceananigans tests right? I think maybe I should add a test that the particle tendencies and sediment models work across all model types. Although again their ability to function should be caught by the Oceananigans tests now. Perhaps I should test their combinations since that's an issue I found recently. |
I forgot I hadn't pushed the sediment model fixes. I've just made this PR #121 which has them so I think we should merge these tests and fixes, then look at adding more sediment model tests in that PR. |
|
||
archs = (CPU(), ) | ||
|
||
@testset "Light attenuaiton model" begin |
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.
@testset "Light attenuaiton model" begin | |
@testset "Light attenuation model" begin |
Pᵢ(x,y,z) = 2.5 + z | ||
model = model_type(; grid, | ||
biogeochemistry, | ||
tracers = unique((required_biogeochemical_tracers(biogeochemistry)..., :T, :S))) # because hydrostatic free surface will request T and S and some BGC models will too |
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.
tracers = unique((required_biogeochemical_tracers(biogeochemistry)..., :T, :S))) # because hydrostatic free surface will request T and S and some BGC models will too | |
tracers = unique((required_biogeochemical_tracers(biogeochemistry)..., :T, :S))) # because hydrostatic free surface model will request T and S and some BGC models will too |
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.
lgtm
Not sure this is the most efficient way todo this, but since
xnode
is defiend differedntly for different grid types this is by far the easiest fix. The alternative would be to definexnode(i, grid, lx, ly, lz)
for RectilinearGrids. Perhaps that solution would be better.Closes #119