smrt-model / smrt

Snow Microwave Radiative Transfert model to compute thermal emission and backscatter from snowpack
Other
49 stars 20 forks source link

Improved way to specify arguments in make_model #33

Open ghislainp opened 2 months ago

ghislainp commented 2 months ago

This is a message sent to the community to obtain its preference.

The make_model function is commonly used in SMRT in its simple form:

make_model("iba", "dort")

However, it is frequent to need some options to the rtsolver ("dort" here), and occasionally to the emmodel ("iba"). The normal way to specify these options using a dictionnary:

m = make_model("iba", "dort", rtsolver_options={"n_max_stream": 128, "m_max": 8})

#or better imho
m = make_model("iba", "dort", rtsolver_options=dict(n_max_stream=128, m_max=8))

This is not easy to read and with current developments in SMRT, a new rtsolver will have non-optional arguments, meaning that rtsolver_options will have to be used all the time (and in addition the term "options" is becoming misleading).

Note that in any case, the current way to specify options will remain operational. The new solutions proposed below are for convenience only.

A first solution to improve readibility is to add a "rtsolver" function that way:

m = make_model("iba", rtsolver("dort", n_max_stream=128, m_max=8))

# which read better with a line break imho:
m = make_model("iba", 
               rtsolver("dort", n_max_stream=128, m_max=8))

the "rtsolver" function is general, it can be used with any rtsolver (dort, nadir_lrm_altimetry, etc...).

Similarly a "emmodel" function would be introduced

m = make_model(emmodel("some_emmodel", someoptions=42),
               rtsolver("dort", n_max_stream=128, m_max=8))

This is more readible but still involves several parentheses. A second solution is to get ride of the make_model function, using an operator (| or + or @ or &)

m = emmodel("some_emmodel", someoptions=42) | rtsolver("dort", n_max_stream=128, m_max=8))

It is light but the function of this line is less clear than the make_model which is very explicit. It is less common. A possible alternative is

model = make_emmodel("some_emmodel", someoptions=42) | make_rtsolver("dort", n_max_stream=128, m_max=8))

Note that this latest choice implies to break some compatibility because make_emmodel is already defined in model.py wiht another meaning, and is used is a few cases. However this usage is infrequent, breaking this compatibility is not critical.

Both solutions are not exclusive, both could be implemented at the same time, but considering the DRY,

Which solution do you prefer ? or do you see alternatives.

pikaqiu2002 commented 2 months ago

I think this type may understand more easily

m = make_model(emmodel("some_emmodel", someoptions=42),
               rtsolver("dort", n_max_stream=128, m_max=8))

Because it's retained the previous method of using make_model, and at the same time let me know that two different types of parameters should be entered in the make_model method.