Skip to content
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

Open
fonsp opened this issue Jan 20, 2021 · 4 comments
Open

Comments

@fonsp
Copy link
Member

fonsp commented Jan 20, 2021

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 containing climate_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" => ...)

@fonsp
Copy link
Member Author

fonsp commented Jan 20, 2021

Proposal 1

We choose a favorite for each category, and use it everywhere

Proposal 2

We 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 code

max_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 code

function 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)

@fonsp
Copy link
Member Author

fonsp commented Jan 20, 2021

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.

@hdrake
Copy link
Collaborator

hdrake commented Jan 20, 2021

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.

@fonsp
Copy link
Member Author

fonsp commented Jan 20, 2021

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 PerControl in our code instead of MRGA, we write code that is general enough to be extended with a fifth control type.

@hdrake hdrake added this to the v1.0.0 release milestone Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants