-
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
Inconsistent naming "M" and "mitigate", nested and flat, struct and Dict #69
Comments
Proposal 1We choose a favorite for each category, and use it everywhere Proposal 2We create a new type, which is a container per technology: Base.@kwdef mutable struct MRGA{T}
M::T
R::T
G::T
A::T
end Example use in codemax_slope = MRGA(1/40, 1/40, 1/80/ 0.0)
asdf = max_slope.M > 0.0
# Base.@kwdef means that you can also use keyword arguments:
enabled = MRGA(M=true, R=true, G=false, A=false)
controls = MRGA(
[0.5, 0.3, ...],
zeros(20),
zeros(20),
zeros(20)
)
controls.R .= 0.1
Example use inside the package codefunction optimize_controls!(
m::ClimateModel;
...
max_slope::MRGA{Float64} = MRGA(1/40, 1/40, 1/80/ 0.0)
...
)
# MRGA is a parametric type (MRGA{T}), which means that you can still enforce a type for its fields:
mutable struct Economics
...
cost::MRGA{Float64}
...
end
mutable struct ClimateModel
...
controls::MRGA{Vector{Real}}
...
end We can add some convenience methods like: MRGA(x) = MRGA(x,x,x,x) |
With proposal 2, we can also add a convenience method for iteration. This is maybe more of a "coolness" factor, because compactness is not always clarity, it's up to you: # we implement two Base methods:
Base.iterate(container::MRGA) = ... something
Base.get(container::MRGA, technology::String) = ... something # so that you can do
for (technology, value) in MRGA(true, false, false, false)
if technology == "R"
@test value == something_else[technology]
end
end
something_else = MRGA(false, false, false, false) One example of something I was writing today, in the current syntax (top), and the proposed syntax (bottom): screenshot. |
Yeah, I think the proposed syntax is better. MRGA is fine for now, but consistent with #44 we will probably want a general "Controls" type, with MRGA being the default subtype used in margo. I imagine some users will want to define some additional or different controls. |
Right! I'll think about it more closely, but my first guess would be that we could do: abstract type PerControl{T} end
mutable struct MRGA{T} <: PerControl{T}
M::T
R::T
G::T
A::T
end and then ideally, by using |
The package has lots of concepts that are 'per technology': M,R,G,A, but their syntax is inconsistent:
Naming
Sometimes we have
M
,R
,G
,A
,sometimes
mitigate
,remove
,geoeng
(abbreviated),adapt
Nested and flat
Sometimes we have nested properties:
climate_model.controls
containingclimate_model.controls.M
,climate_model.controls.R
, etc.,and sometimes it is a flat structure:
climate_model.economics.mitigate_cost
,climate_model.economics.remove_cost
, etc.Struct and Dict
For nested structures, sometimes we use a struct:
climate_model.controls.M
and sometimes we use a
Dict
:optimize_controls!(m; max_slope=Dict("mitigate" => ...)
The text was updated successfully, but these errors were encountered: