Change SoilModel to use the data object

[jacobcook1995]

Description

This pull request alters the SoilModel so that it uses the data object to initialise itself and updates the data object as it goes. As part of this, the soil carbon pools are no longer attributes of the SoilCarbonPools class, instead these soil carbon pools are only stored as part of the data object. This is mainly for the sake of code clarity, as changes to the attributes (initialised from data) would change the data object for everyone. This still happens now but the code is explicit about it happening.

This PR also sets the logger level used by pytest has been to DEBUG globally, in order to ensure that debug messages are always seen by pytest. Previously, they were seen if pytest was run on the entire repo, but not if it was run on individual testing files. This option can be overridden in individual test functions or files, but if it proves to be problematic we can set it to another level.

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist

• Make sure you've run the pre-commit checks: $ pre-commit run -a
• All tests pass: $ poetry run pytest

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov-commenter]

Codecov Report

Merging #187 (55ad0b1) into develop (6c0d5c1) will increase coverage by 0.69%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #187      +/-   ##
===========================================
+ Coverage    95.20%   95.90%   +0.69%     
===========================================
  Files           23       25       +2     
  Lines         1106     1220     +114     
===========================================
+ Hits          1053     1170     +117     
+ Misses          53       50       -3     

Impacted Files	Coverage Δ
virtual_rainforest/models/soil/carbon.py	100.00% <100.00%> (ø)
virtual_rainforest/models/soil/soil_model.py	100.00% <100.00%> (+4.16%)	⬆️
virtual_rainforest/models/abiotic/__init__.py	100.00% <0.00%> (ø)
virtual_rainforest/models/abiotic/wind.py	100.00% <0.00%> (ø)
virtual_rainforest/models/abiotic/abiotic_tools.py	100.00% <0.00%> (ø)
virtual_rainforest/models/abiotic/radiation.py	91.48% <0.00%> (+2.01%)	⬆️
virtual_rainforest/core/base_model.py	100.00% <0.00%> (+2.80%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jacobcook1995]

Oh yeah it's probably worth mentioning that the way that the data object gets updated is a bit of a placeholder. At the moment the value is simply overwritten, down the line we either want to output data at each step (i.e. before it gets overwritten), or want to make the data object into a time series.

[alexdewar]

It looks good from a cursory glance, but I'm not sure whether you said you were still having the problem of clobbering existing values in Data? If so, I guess you'll need to solve that before merging.

[jacobcook1995]

In terms of the data update question, I've now set it up so that a single function updates the data object (SoilCarbonPools.update_soil_carbon_pools), and when it does so it updates the data object explicitly (rather than changing an attribute that was populated from the data object).

Down the line I might move this function into the soil_model module, so that soil model updates to the data object only happen within a single function, but that feels like too big a restructure to make midway through the review process of a PR.

[vgro]

LGTM :-)

[davidorme]

LGTM - one minor question.

The only other thing is that I'd imagined lower level functions mostly passing around numpy arrays and with Dataset and DataArray only really being used when adding to the data object. There's probably a minor speed advantage, but I don't think we should change this here - they're functionally arrays - but we might want at some point to align where (or indeed if) we change from DataArray to NDArray inputs.

[davidorme]

It seems a little odd to have each variable passed in as a dataarray , except for maom and lwmc, which are wrapped up within data still? Is there any reason not to pass those arrays in?

[jacobcook1995]

This is basically an artefact of maom and lmwc previously being attributes of SoilCarbonPools. When they stopped being attributes I changed how they were accessed without making them consistent with what had gone before.

However, I am tempted to leave them as is, as the way this data is accessed has already been changed on my integration branch. So, it might be more productive to have the DataArray vs NDArray discussion when the pull request for that is in?