OceanBioME / OceanBioME.jl

🌊 🦠 🌿 A fast and flexible modelling environment written in Julia for modelling the coupled interactions between ocean biogeochemistry, carbonate chemistry, and physics
https://oceanbiome.github.io/OceanBioME.jl/
MIT License
40 stars 20 forks source link

Overhaul of box model to use more of the Clima infrastructure + data assimilation example #164

Closed jagoosw closed 7 months ago

jagoosw commented 8 months ago

This PR now overhauls BoxModel to 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.

https://github.com/OceanBioME/OceanBioME.jl/assets/26657828/0f7949a3-3e62-4fd4-8a9b-1372c77c0f67

CC: https://github.com/orgs/OceanBioME/discussions/163

jagoosw commented 8 months ago

I'm not sure its worth making this a regular example but it doesn't take too long to run

jagoosw commented 8 months ago

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.

jagoosw commented 8 months ago

Hmm this is strange, the docs build normally when I run them manually on kelp

jagoosw commented 8 months ago

@glwagner I've made quite a few changes so that BoxModel is now an AbstractModel and reuses all of the stuff in Oceananigans for simulations etc. as far as possible.

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.

jagoosw commented 8 months ago

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.

jagoosw commented 7 months ago

Not sure why the docs are failing with the model implementation page being too large, when I build locally its only 31KB so something must be failing

jagoosw commented 7 months ago

Ah JLD2 is throwing a warning over and over: https://oceanbiome.github.io/OceanBioME.jl/previews/PR164/model_implementation/

jagoosw commented 7 months ago

Now thats fixed but there is a new completely random error which is strange.

navidcy commented 7 months ago

OK, I think the main package tests was using Oceananigans v0.90.3 and the docs were using v0.90.6. That's why the "weird" error in the docs. Now I updated the Oceananigans version for the main package and we get the same "weird" error there!

At least there is consistency. Not sure yet what causes that...

navidcy commented 7 months ago

Seems something related to interpolation... Might be a bug that was introduced by https://github.com/CliMA/Oceananigans.jl/pull/3395 that Oceananigans tests do not cover.

cc @glwagner

navidcy commented 7 months ago

I think I found the culprit; see 45177f0

navidcy commented 7 months ago

@jagoosw see if any other syntax was changed, e.g. ,_interpolate, that needs updating. Alternatively we can add an upper bound on Oceananigans up to 0.90.5 and deal with syntax changes later. But if you decide this add the bound both in the Project.toml + docs/Project.toml.

jagoosw commented 7 months ago

Oh yeah thank you! I'll update the interpolation syntax

jagoosw commented 7 months ago

I've sorted the interpolations issue now @navidcy

jagoosw commented 7 months ago

Also I think the warning is from the output writer trying to serialise the $\kappa$ function which it can't (https://github.com/CliMA/Oceananigans.jl/issues/2245)

navidcy commented 7 months ago

good for fixing tests! docs now fail at https://buildkite.com/oceanbiome/oceanbiome/builds/106#018d5c26-6676-4287-bdd7-0c89cf33c619/26-2397

jagoosw commented 7 months ago

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?

navidcy commented 7 months ago

Sure. Why not. We can always improve it later.

jagoosw commented 7 months ago

Great, I'll merge