Variables 1 vgro

[vgro]

I reviewed the variables in data_variables.toml that are related to the abiotic models and the hydrology model. I also updated the contents of required_init_vars, vars_updated, required_update_vars and vars_initialised. I hope I captured everything.

I have not updated the tests because I was not quite sure if that was sensible at this stage.

[dalonsoa]

A step in the right direction, but there are a few things to change. You have A LOT of variables!!!

[vgro]

A step in the right direction, but there are a few things to change. You have A LOT of variables!!!

Thanks for the feedback.
I agree that I have a lot of variables, however, they do cover three different models ;-)
I had not delete the ones that are in other models from this list as I thought this will all be merged in the end. I did that now that for clarification.
I think a lot of the variables I listed will not be interesting for anyone other than me to actually look at in the end. It is very convenient to store them in the data object to make them accessible for different models and functions, but I am happy to adopt an alternative approach that takes the weight off the data object and variable system.

@davidorme I am using "topofcanopy_radiation" as input which I think you call "shortwave_in"? And I use "canopy_absorption" for what you call "layer_absorbed_irradiation". Do you have any reasons for picking one or the other, like pyrealm dependencies? Then I'm happy to rename it in my models. I expect "stomatal_conductance" as an input from the plant model, is that what your outputting?

[davidorme]

@vgro - I think it's easier if you don't delete the ones that aren't yours. Sorry! Otherwise we'll be in a position of having multiple files to remerge - that would be fine if we didn't share variables but we'll get dupes!

[davidorme]

I'm happy with those name changes!

[vgro]

@vgro - I think it's easier if you don't delete the ones that aren't yours. Sorry! Otherwise we'll be in a position of having multiple files to remerge - that would be fine if we didn't share variables but we'll get dupes!

haha ok, no problem, I'll add them back in

[davidorme]

Super - thanks. I might work directly on this branch, if that's okay with you - easier to see what has been fixed up already!

[vgro]

@davidorme all the variables that you added are already in the toml file but the name is different.
air_conductivity is called air_heat_conductivity, I fixed that in #430 but it is probably not merged in this branch.
The accumulated fluxes are call surface_flux_accumulated and subsurface_flow_accumulated, did you take the names from the docstring? Might be misleading so maybe it is better to change the docstring.

[davidorme]

@vgro - they all got picked up from running them through the system to discover all variable usage by models. I think they're all just instances where the model variable lines need deleting or changing?

virtual_ecosystem/virtual_ecosystem/models/abiotic/abiotic_model.py

Line 66 in 86656f7

"air_conductivity",

virtual_ecosystem/virtual_ecosystem/models/hydrology/hydrology_model.py

Lines 112 to 113 in 86656f7

	"accumulated_surface_runoff",
	"accumulated_subsurface_flow",

Should we just update the names in those lines and then delete the variables I've added to data_variables.toml?

[vgro]

@vgro - they all got picked up from running them through the system to discover all variable usage by models. I think they're all just instances where the model variable lines need deleting or changing?

virtual_ecosystem/virtual_ecosystem/models/abiotic/abiotic_model.py

Line 66 in 86656f7

"air_conductivity",

virtual_ecosystem/virtual_ecosystem/models/hydrology/hydrology_model.py

Lines 112 to 113 in 86656f7

	"accumulated_surface_runoff",
	"accumulated_subsurface_flow",

Should we just update the names in those lines and then delete the variables I've added to data_variables.toml?

Like I said, I have already renamed the air_conductivity to air_heat_conductivityin #430, I don't know why this still shows the old name. For the accumulated fluxes, yes, name change is probably best. The names are correct in vars_updated. I can do that later or you can do it if you are still making changes, I'm on the stats at the moment

[davidorme]

@dalonsoa This PR is in an odd place. I've checked things out with local merges and variables_1_vgro now includes all the variables needed for the variables system to run. I've made some changes to variables_1 to update expected required_init_vars formats and to fix some other minor issues with the variables system and with our existing codebase.

I haven't aligned everything (some tests fail on their expected log output), but I think it's going to be much easier to merge this down at this point (even if we have to bypass protections) and then work on a single PR.

I could merge variables_1 up into this though. What is your preference?

ETA: Actually - damnit - I'm going to have to merge variables_1 in to resolve merge conflicts before we can do anything. I'll do that now.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (variables_1@4e477d5). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff               @@
##             variables_1     #431   +/-   ##
==============================================
  Coverage               ?   94.62%           
==============================================
  Files                  ?       71           
  Lines                  ?     3886           
  Branches               ?        0           
==============================================
  Hits                   ?     3677           
  Misses                 ?      209           
  Partials               ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dalonsoa]

I think all looks good to me. This includes the changes to all models?

[davidorme]

I think all looks good to me. This includes the changes to all models?

It does include all models. Tests all now pass - I have updated all the code and tests to handle the simplified required_init_vars structure and added a couple of small bits.

The docs fail for relevant reasons - the docs now include a walk through of the model running on the example data. That is using a wider set of models than are included in test_ve_run and that has revealed some variables issues - the "initialised by update" variables that we've mentioned elsewhere. So - those need resolving still.

I think we probably still merge this down to have one branch on this issue again and tackle that problem separately?

[dalonsoa]

Yes, I think that's the best way forward.