Feature/co2 dummy

[vgro]

This is a very simple atmospheric CO2 model which currently just copies ambient CO2 to all atmospheric layers and adds or subtracts what plants, soil microbes, animals produce/use. There is no mixing implemented and the vertical axis might need adjustment after discussion on structure next week.

I didn't write tests for most of the helper functions because they are just simple sums, might not even be necessary to have them as functions. I'm looking forward to your comments on this :-)

Also, @davidorme I don't know if the canopy layers respond to different CO2 levels or if the canopy could be treated as one bulk?

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 #193 (6ff7548) into develop (cec0f9c) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #193      +/-   ##
===========================================
+ Coverage    96.01%   96.15%   +0.13%     
===========================================
  Files           26       27       +1     
  Lines         1206     1248      +42     
===========================================
+ Hits          1158     1200      +42     
  Misses          48       48              

Impacted Files	Coverage Δ
virtual_rainforest/models/abiotic/__init__.py	100.00% <ø> (ø)
...rtual_rainforest/models/abiotic/atmospheric_co2.py	100.00% <100.00%> (ø)

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

[jacobcook1995]

Looks generally sensible to me.

I do think that the helper functions should be tested though. 2 of them raise exceptions and use more complex data types and I think that behaviour should be tested. And one of them subtracts which is probably worth testing for as it could plausibly cause a sign error down the line.

Don't personally see the need for testing the function that only adds three values together, but not actually sure if that needs to be a separate function as it could be a single line in the higher level function. What do you think @davidorme?

[davidorme]

Looks good .

I agree with @jacobcook1995 about testing the simple functions, but also that those one line functions may be splitting the code too finely. There are costs to calling functions and since these are all simple sums, pulling it all back together into the main function until the mixing, which will get more complex, might be easier.

The plants will definitely be able to take account of per layer CO2 concentrations and probably should do so.

[davidorme]

I don't think this will be a constant - it will be input data.

[davidorme]

Oh - ok. I see that it is a placeholder in case the data isn't provided. I'd just have this as a default argument to calculate_co2_profile, although in general, I think we should probably make people explicitly provide the data rather than internally setting defaults.

[vgro]

We do want people to provide input but CO2 is something that is likely not going to change much if the simulations are shortish. I followed the same structure as the other modules here, putting the default values in the constants class and then call them to make sure we don't have multiple default values across the code.

[davidorme]

This is not robust to the required shape of the array - it only produces a 1D array. It should probably be something like:

np.ones_like(something_with_the_right_shape) * co2_const.ambient_co2_default

[vgro]

This has to be a 1D array as it creates an input co2 concentration for each cell if not provided in data, so it is correct. This might become outdated if I drop the check discussed in the comment above.

[davidorme]

Right! Sorry - this is the ambient atmospheric (above canopy?) CO2, so is not expected to vary except possibly spatially, which is what you have.

[vgro]

I changed the name from ambient_co2 to atmospheric_co2_topofcanopy to make that clearer

[vgro]

@jacobcook1995 and @davidorme I incorporated your suggestions or commented for more explanation. There are two points that I'm not sure about:

1. How can I set defaults that are DataArrays in the needed shape without the not in check that I used at the beginning of the class init?
2. I agree that simple sums don't need to be functions, however, when I wrote the tests as suggested in turned out that the dimensions can cause problems. Could be picked up by testing the class but I think I'll leave it as it is for now while still in development?

many thanks :-)

[jacobcook1995]

LGTM! I don't think there's any problem with having very simple functions really so long as they are tested

[davidorme]

One minor possible simplification, otherwise all looks good.

On the default data arrays, I would probably lean towards insisting that they exist and using those values as the inputs without providing any defaults. That way this function just does the calculation.

Ultimately, making sure that CO2 and all the rest exist is a job for something further up the stack. Some of that might be defaults in the configuration, some might be spinning up the models.

I'm not sure we should worry about that now, but I think the checking is ultimately in the wrong place?