System to allow constants to be altered based on configuration #258
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #258 +/- ##
===========================================
+ Coverage 95.08% 95.31% +0.23%
===========================================
Files 42 44 +2
Lines 1708 1794 +86
===========================================
+ Hits 1624 1710 +86
Misses 84 84
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Is there a way (or really a need) to provide some kind of a base model for constants classes? We could even build a registry and have some kind of checking at startup? This isn't critical for this proof of concept at all, but it would be nice to be able to enforce some kind of structure on the constants classes. |
I'm not sure about a base model, because would the different constants classes share any attributes or methods? It might be worth adding them to a registry though for ease of access though |
|
Yeah - it isn't an ABC, something simpler that just checks for "frozen dataclass"-ness and registers them. But that really isn't needed here or even for rolling this out to other models. |
|
@vgro I've just added a new docs page that hopefully addresses some of the problems you pointed out. Let me know if it looks sensible to you, and if there is anything you think is missing |
| # Virtual Rainforest parameterisation | ||
|
|
||
| The Virtual Rainforest contains a very large number of constants. These constants are |
There was a problem hiding this comment.
I think this file mixes user facing information and model creation information. The user-facing stuff is fairly short, but this is far more detailed than is needed here. I suspect we may need to break up the creating a model section into multiple pages, but we can park that.
If the structure of the constants classes etc moves into new model creation, we can then be more specific about details: add a type, because otherwise dataclass treats it as a class attribute, rather than a instance attribute.
We also should think about what other user facing help we want here. I can imagine that we might add an export_model_constants class method to the BaseModel which just spits out a TOML document from any model of the constants. I don't know if that is something we need to put in place for @dalonsoa?
That could then be exposed via a command line endpoint: vr export_model_constants plants plant_constants.toml.
What users need to know in this document is how constants are used, how to find what they can change and how to change it in TOML. I'm also wondering if calling it constants.md might be better 😄
| # Import dataclass of interest | ||
| import_path = f"virtual_rainforest.models.{model_name}.constants" | ||
| consts_module = import_module(import_path) | ||
| ConstantsClass = getattr(consts_module, class_name) |
There was a problem hiding this comment.
I think a constants class registry is the way forward here. It isn't needed for the proof of concept, but might be worth implementing now? If not, we should add as an issue. I guess we'd key by model name and constants class name, so we could go:
constants_class = CONSTANTS_CLASS_REGISTRY[model_name][class_name]
David's simplification of the check Co-authored-by: David Orme <[email protected]>
There was a problem hiding this comment.
Nothing to add. It looks a good approach easy to roll out to the other models.
My only concern is that the constants that a model requires and their default values are going to be hard to find except if the interested party dives into the code or the documentation for the API, which is less than ideal.
Probably not for this PR, but exploiting the idea of the constants registry suggested by @davidorme , I guess it should be possible to extract all the constants and their values and put them in a dedicated section in the documentation when this one is built.
@alexdewar , here you have all you need to start working on the sensitivity analysis code!
|
Ah great! I'll add an issue for the constants registry + documentation |
|
@davidorme I've addressed you requested changes aside the constants registry one, which I've made an issue for (#259) |
|
LGTM |
Description
This PR adds a system for reading constants from the configuration, which is necessary for sensitivity analysis.
The idea is for each model to define either one or a small number of data classes in its
constants.pymodule, with units and default values recorded there. Input arguments for these data classes can be stored in the configuration, and supplied in order to overwrite default values. This takes place within the model'sfrom_configfactory method, generating instances of the constants classes to be fed to__init__, either using the default values, or configured with user provided values as needed.The model
__init__arguments are then extended to include any constants classes required and supplied constants class instances are then stored as attributes of the model instance and can be passed on to downstream functions either as entire constants classes or unpacked to individual members as required.The new system is only implemented here for the
abiotic_simplemodel. The plan is that once it's merged further pull requests should be made to switch the other models (as well as core see #129) over to this system.Anyway let me know what you think of this first draft.
Fixes #209
Fixes #210
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks