Overhaul of box model to use more of the Clima infrastructure + data assimilation example #164
Conversation
|
I'm not sure its worth making this a regular example but it doesn't take too long to run |
|
Its also interesting to not how insensitive this model is/how made up the parameters can be as the "learned" values are quite different from the "truth" ones, but for this case replicate the data well. I guess for a real-life case this indicates you do need more varied data/scenarios. |
|
Hmm this is strange, the docs build normally when I run them manually on kelp |
examples/data_assimilation.jl
Outdated
|
|
||
| @inline PAR(t) = PAR⁰(t) * exp(- 0.2 * 10) | ||
|
|
||
| function model(initial_photosynthetic_slope, |
There was a problem hiding this comment.
This code a little harder to read because this function is called model, but there is also an object below called model = BoxModel(...). For examples I think it's good to be abnormally verbose, both for understandability and because a lot of code will end up being based on the example, so it has an outsized influence on coding style in the community.
There was a problem hiding this comment.
Maybe following the style from other examples, e.g. simulation = Simulation(..., you could do boxmodel = BoxModel(....
There was a problem hiding this comment.
I'd advocate changing the name of the function to precisely reflect either the action taken, or the values returned. Functions fall broadly into two categories: verbs (actions) and nouns (constructors --- technically, a constructor takes an action that results in the construction of an object, but our convention is to use "nouns" for constructors).
In this case the function runs a simulation and returns P, times. So I don't think that "model" is the right name for the function. Perhaps "run_box_simulation" or something.
There was a problem hiding this comment.
Thats a good point, thank you!
I will change to something like run_box_simulation.
examples/data_assimilation.jl
Outdated
| P = zeros(1093) | ||
|
|
||
| for (idx, t_idx) in enumerate(length(times)-1092:length(times)) | ||
| P[idx] = file["values/$(times[t_idx])"].P |
There was a problem hiding this comment.
Why does the "time" have a field .P?
There was a problem hiding this comment.
P is a property of a tuple of values saved as values/timestep index like how in FieldTimeSeries we have timeseries which then has timesteps and in each timeseries/time step index you get each tracer.
There was a problem hiding this comment.
Why not use the file address timeseries/iteration/name here?
There was a problem hiding this comment.
Good question, I'll change that for consistency too
jagoosw
changed the title
|
@glwagner I've made quite a few changes so that I guess it might be more appropriate for the box model to not be part of this package as its not really specific to the BGC stuff at all but we can move in the future e.g. when the simulation stuff gets moved out of Oceananigans. |
|
I'm just testing the updates to the data assimilation example now but think it may be a bit expensive to be worth running for the docs. The box model stuff is a bit slower now, I would guess the most expensive bit is that now instead of just writing a bunch of numbers to the file its got to write fields. |
|
I've sorted the interpolations issue now @navidcy |
|
Also I think the warning is from the output writer trying to serialise the |
|
good for fixing tests! |
|
Yay, it finally built and only took 15ish min longer to run. The example is here https://oceanbiome.github.io/OceanBioME.jl/previews/PR164/generated/data_assimilation/ Do we think this is worthwhile including? |
|
Sure. Why not. We can always improve it later. |
|
Great, I'll merge |
This PR now overhauls
BoxModelto use the Clima infrastructure (and breaks the box model API in the process).This is a simple data assimilation example. Although it's a fictitious case hopefully it shows how it could be used.
evolution.mp4
CC: https://github.com/orgs/OceanBioME/discussions/163