Replies: 2 comments 1 reply
-
I'd really appreciate a user perspective on this - it is going to break older code, but we're already going to need to do that to fix existing issues. This seems much more logical to me. |
Beta Was this translation helpful? Give feedback.
0 replies
-
@davidorme I think the final workflow looks much clearer than previous version! Just checking on one thing: the soil moisture function, as some of the soil moisture function (and perfaps in the future) is wrapped into PModel that modifies beta for example, and I wonder how these would fit into PModelEnvironment? |
Beta Was this translation helpful? Give feedback.
1 reply
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
Given we have breaking changes in the API, we might as well think big. Here are some proposals:
PModelEnvironment
At the moment this doesn't include
fapar
andppfd
. However if we dropPModel.estimate_productivity
to align the signatures and usage ofPModel
andSubdailyPModel
then providing these viaPModelEnvironment
is more natural and makes it easier to swap model settings.PModelEnvironment
then provides all the forcing data. We can allowppfd
andfapar
to default to unity.We have a bunch of "optional" variables, required by some downstream methods. At the moment this is really inflexible, because each has to be written into
PModelEnvironment
. The whole point in using registries for the methods is that people can define new ones, but if they require new forcing data variables, they can't work without changing thePModelEnvironment
definition. So, we should just**kwargs
and then check allkwargs
are appropriately sized numpy arrays. The only thing we lose is bounds checking on the alternate variables but I think the flexibility makes sense. We can then usesetattr
to expose arbitrary variables to downstream methods.The PModelEnvironment should provide
tk
andtk_ref
. A large number of functions use this and are currently calculating it internally - we should change them all to assumingtk
and then calculate it once.FastSlowScaler
In a similar vein, there are a lot of arguments to
SubdailyPModel
that are to do with the acclimation model to be used. I think we get rid ofFastSlowScaler
and build the same functionality into a broaderAcclimationModel
class. That again makes it easier to swap in alternative settings to theSubdailyPModel
and bundles related functionality better.Something like:
PModel and SubdailyPModel
functools.cached_property
to support properties that aren't required as part of the model calculation, rather than the currentself._var
and@property
setup. This is very much simplified inPModel
if we dropestimate_productivity
and providefapar
andppfd
in theenv
, because then we don't need to test for that having been run.With these changes, the signatures will end up like:
Workflow
This would give the following workflow sketch:
I think this is all a lot more logical and clean for users, but they are bigger changes.
Beta Was this translation helpful? Give feedback.
All reactions