-
Notifications
You must be signed in to change notification settings - Fork 13
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
Cleanup data structures #14
Comments
Hi! About 4: I think the current structure is easy to understand and work with - I do have some minor notes:
|
Hi @fonsp, thanks for the suggestion. This was definitely just me be sloppy early on and now thinking of a better fix, such as what you propose above. I am busy for the next few hours but will try to make the necessary changes today. A similar problem is the specification of the feedback parameter and climate sensitivity. Currently, the climate sensitivity is treated as a "diagnostic" quantity that is evaluated upon construction of the mutable struct Physics
CO₂_init::Float64
δT_init::Float64
a::Float64
B::Float64
Cd::Float64
κ::Float64
r::Float64
ECS::Float64
τd::Float64
function Physics(CO₂_init, δT_init, a, B, Cd, κ, r)
FCO₂_2x = a*log(2) # Forcing due to doubling CO2 (Geoffrey 2013)
sec_per_year = 60. * 60. * 24. * 365.25
ECS = (FCO₂_2x*sec_per_year)/B # [degC]
τd = (Cd/B) * (B+κ)/κ # [yr]
return new(CO₂_init, δT_init, a, B, Cd, κ, r, ECS, τd)
end
end As in the case of |
Right! You could make those dependent parameters ( Don't feel rushed to fix it today - I can work around these minor points. 👍 |
About the model default values, eventually I want to move away from hard-coding this anywhere, whether in For example, I imagine you could email me your run-time parameter --- # Physical parameters
a: 4.97
B: 1.17
... and I could just run: ClimateModel(parameter_file = "fonsp_favorite.yml") or something similar. I think this would be really complimentary to have the defaults directly in the constructor as you suggest, because then we can set it up such that if any parameters are left out of the YAML / CSV file, then it can just take the hard-coded default value. |
That is really convenient! It would be great to have human-readable and easily modifiable input files that uniquely determine a MARGO configuration! |
I've started Julia-fying the package structure and submodel data structures in this branch https://github.com/hdrake/ClimateMARGO.jl/tree/updating-structure, which is still very much a work in progress. Interpretable and convenient functions for diagnostics
T(T0, F, Cd, κ, B, t, dt; A=0.) = sqrt.(1. .- A) .* (
T0 .+
T_fast(F, κ, B) .+
T_slow(F, Cd, κ, B, t, dt)
) and T(model::ClimateModel; M=false, R=false, G=false, A=false) = T(
model.physics.T0,
F(model, M=M, R=R, G=G),
model.physics.Cd,
model.physics.κ,
model.physics.B,
t(model),
model.domain.dt,
A=model.controls.adapt .* (1. .- .~future_mask(model) * ~A)
) Re-tooling dependent parameters as diagnostic functions ECS(a, B) = F2x(a)/B
ECS(model) = ECS(model.physics.a, model.physics.B) @fonsp, what do you think? If you like it, I'll continue re-implementing all of the other diagnostics. I think it makes the code much more readable, user-friendly, and bug-repellent. |
A lot of this has now been implemented in #19, but it is still a work-in-progress. Will close this Issue once that is merged. |
Before tackling documentation #2 and unit tests #3, the first priority is to cleanup the data structures (see branch https://github.com/hdrake/ClimateMARGO.jl/tree/data-structure-cleanup).
There are a few features which are particularly undesirable about the current data structures:
src/optimization/jl
, instead choosing to write out the objective function in its entirety, which is excessively long and susceptible to bugs. My guess is that it is possible to write the objective condition in one line that looks something like (in pseudo-JuMP syntax)where
M
,R
,G
,A
are the control variables.2. The few functions that are currently used in the JuMP optimization code have explicitly re-defined within the optimization code, probably for legacy reasons. Ideally, the diagnostic functions used for plotting / analysis should be re-used for the optimization to reduce the likelihood of discrepancies between the two versions of the code (which indeed caused breaking bugs in early versions of the implementation).
3. There are a lot of redundant functions that should be collapsed into a single function with keyword arguments and/or different methods.
4. We should rethink the
ClimateModel
,Physics
,Controls
, andEconomics
structs. They are fairly unwieldy because I did not really understand Julia constructors when I created them half a year ago. It might be useful to look at how more mature julia models like https://github.com/CliMA/Oceananigans.jl or https://github.com/CliMA/ClimateMachine.jl structure their submodules for inspiration.The text was updated successfully, but these errors were encountered: