Restructure of BaseModel initialisation flow#1193
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1193 +/- ##
===========================================
- Coverage 94.96% 94.95% -0.02%
===========================================
Files 71 71
Lines 7273 7288 +15
===========================================
+ Hits 6907 6920 +13
- Misses 366 368 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dalonsoa
left a comment
There was a problem hiding this comment.
Looks nice. Definitely more consistent across the code. Happy to unmock the mocks in the tests, as that is my mess.
|
@dalonsoa I realised that the class attributes section of the "defining_new_models.md" process was woefully out of date. I've updated it and gone into some detail about the resolution of execution order and the core Could you give the most recent commit 5f90307 a quick look to see if it all makes sense? |
dalonsoa
left a comment
There was a problem hiding this comment.
Aside from my comment, which if addressed should be done so in a separate PR, this looks good.
Description
This PR switches over to the more explicit sequencing of the model
__init__described in #1191:This:
_setup.kwargscomponents of the BaseModel arguments and has an explicit list of model arguments. This identified a couple of testing issues where old model arguments were still in test code, becausekwargslets them sneak through.__init__signature.To do this:
I've lightly modified the BaseModel, including changes to the static flagging functions to set model attributes rather than passing around boolean return values. That has included inverting
_bypass_setupto_run_setupto sit alongside_run_updateand to give the more intuitiveif self._run_setup:above.Updates all of the model files to the new structure above. I've re-ordered the sequence of methods to be consistent (
__init__,_setup,from_config) across models.Updates the class docstrings to actually document all of the constructor args. It would be nice if
ruffvalidated that class docstrings contained all the__init__args, but that doesn't seem easy to do. May be we should move Args inside__init__.Provides some minor fixes to the tests:
_run_setupvalue from_bypass_setup(True<->Falseswaps)__init__Important
This also revealed a bigger issue with the use of patching of the static settings in model
__init__and_setuptesting, which was masking some missing init vars.I think this is being done because the tests use compiled sets of test data that include variables created by init and update, so the static settings were being patched to avoid raising configuration errors. That's a problem because it breaks the explicit variable checking - for example, in the abiotic model it was masking incomplete required vars lists - the variable might not be in the required list, but is in the data anyway, so the test runs.
I've fixed one case here because it was the easiest way to get tests to pass, but we should separately remove all of this patching and simply subset the more inclusive datasets down to the actual alleged required variables.
Fixes #1161
Type of change
Key checklist
pre-commitchecks:$ pre-commit run -a$ poetry run pytestFurther checks