Various bug fixes to allow `vr_run` to work with `soil` and `abiotic_simple` models by jacobcook1995 · Pull Request #236 · ImperialCollegeLondon/virtual_ecosystem

jacobcook1995 · 2023-06-07T10:12:12Z

Description

This pull request is mainly small bug fixes to make the vr_run function runable for the abiotic_simple and soil models (using data stored on RDS).

The most substantive change is moving the definition of layer roles from the abiotic_simple model into the core utilities. This was necessary as the soil model also needed to make use of these layers roles (to find the index associated with the top soil layer).

This PR also updates the documentation to explain to users how to access the data (if they are part of the VR project), and updates the simulation flow documentation to reflect recent changes to the flow.

Fixes #235

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

…soil model

…ollegeLondon/virtual_rainforest into bugfix/vr_run_problems

…ading them from upstream input

…ta to usage docs

jacobcook1995 · 2023-06-07T10:12:59Z

Adding @vgro as a reviewer as I'm not sure of best practice when a pull request is cowritten

codecov-commenter · 2023-06-07T10:19:11Z

Codecov Report

Merging #236 (6ce98b8) into develop (299e39b) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #236      +/-   ##
===========================================
+ Coverage    95.23%   95.33%   +0.10%     
===========================================
  Files           36       40       +4     
  Lines         1553     1715     +162     
===========================================
+ Hits          1479     1635     +156     
- Misses          74       80       +6

Impacted Files	Coverage Δ
virtual_rainforest/core/data.py	`88.60% <ø> (ø)`
virtual_rainforest/core/utils.py	`97.56% <100.00%> (+2.32%)`	⬆️
...rest/models/abiotic_simple/abiotic_simple_model.py	`100.00% <100.00%> (ø)`
...nforest/models/abiotic_simple/simple_regression.py	`100.00% <100.00%> (ø)`
virtual_rainforest/models/soil/soil_model.py	`100.00% <100.00%> (ø)`

... and 8 files with indirect coverage changes

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

vgro

LGTM

davidorme

LGTM - minor comment on using admonitions.

I think we should structure the main simulation document a bit more (and introduce all of the defined steps, even if they are currently lightly used). I was going to make suggestions, but instead I'm going to commit a suggested update. I've updated the Sphinx conf to include a mermaid diagram. I think that is useful here and it is probably something we'll want to use more of.

davidorme · 2023-06-08T09:20:32Z

docs/source/virtual_rainforest/core/data.md

+**NOTE**: At the moment,
+`core.data.variable` tags cannot be used across multiple toml config files without
+causing `ConfigurationError: Duplicated entries in config files: core.data.variable` to
+be raised. This means that all variables need to be combined in one `config` file.
+


I think it is worth explicitly using warning admonitions here. Partly that is that they look nice and call more attention to the warning, but it is then also easier to search for and review these kinds of messages.

Suggested change

**NOTE**: At the moment,

`core.data.variable` tags cannot be used across multiple toml config files without

causing `ConfigurationError: Duplicated entries in config files: core.data.variable` to

be raised. This means that all variables need to be combined in one `config` file.

```{warning}

At the moment,

`core.data.variable` tags cannot be used across multiple toml config files without

causing `ConfigurationError: Duplicated entries in config files: core.data.variable` to

be raised. This means that all variables need to be combined in one `config` file.

```

On another note - Myst has relatively recently gone to version 1.0.0 and there are some stabilised syntax things like using colon-fence notation. We should probably go through and update docs to use the recommended 1.0.0 syntax.

Actually it looks like we're currently pinned to myst-parser <1.0.0 by some myst-nb and sphinx version pins ( see executablebooks/MyST-NB#513). As and when that is resolved we should review our MyST content and syntax.

jacobcook1995 · 2023-06-08T11:08:02Z

docs/source/virtual_rainforest/main_simulation.md

-that setting is not found in the configuration, but if no default is set then the
-validation will fail. Further details can be found in the [configuration
-documentation](./core/config.md).
+* [`AbioticModel`](../api/abiotic.md),


I think this probably should be abiotic_simple.md as that's the version we are using at the moment

Good point - I've included both links since I think they will both be here to stay.

jacobcook1995 · 2023-06-08T11:09:21Z

docs/source/virtual_rainforest/main_simulation.md

-procedures to ensure that the loaded data can be mapped onto the spatial grid and also
-other core dimensions for the configured simulation. Further details can be found in the
-[data system](./core/data.md) and [core axes](./core/axes.md) documentation.
+The core configuration will include the configuration details for each individual


These details won't be in the core configuration

Yeah - clumsily phrased.

vgro · 2023-06-13T07:55:05Z

LGTM - minor comment on using admonitions.

I think we should structure the main simulation document a bit more (and introduce all of the defined steps, even if they are currently lightly used). I was going to make suggestions, but instead I'm going to commit a suggested update. I've updated the Sphinx conf to include a mermaid diagram. I think that is useful here and it is probably something we'll want to use more of.

I like the idea of using the diagrams, however, they don't come out very nicely at the moment (e.g. the lines go over the text).

davidorme · 2023-06-13T08:05:10Z

Yeah - it definitely isn't as I'd like and I am normally really fussy about figures. That is mostly the tool: mermaid doesn't really do fine controls - or at least not without lots of tweaking of things. It does produce diagrams quickly though! We could replace this with a hand crafted image that we're happier with when things are stable, but this is quick to edit and update for now?

vgro and others added 20 commits June 6, 2023 10:17

ini soilmoisture changed to float

7be2a0d

Removed soil mositure and temperature as required init variables for …

838c044

…soil model

upper time bound changed to 1 month

6c53e04

Removed outdating logging info from tests

15ccd7e

Merge branch 'bugfix/vr_run_problems' of https://github.com/ImperialC…

5dec56c

…ollegeLondon/virtual_rainforest into bugfix/vr_run_problems

layers moves to core schema, not tested

1bcd22d

schemer updated for layers in core

a3492da

abiotic simple config updated layers from core

7da06b3

layer roles function moved to utils

06484c4

Updated set_layer_roles docstring to include Raises

84c6fc7

Switched the soil model to generate layer_roles itself rather than re…

4ba300d

…ading them from upstream input

time_index updated

791873f

soil temperature added to setup

fe56e19

Changed default moisture value to be within acceptable range

68bcb82

note on core.data.variables in docs

bce650c

updated soil moisture unit in abiotic simple

700f097

corrected time coordinates

3e5d7b1

Added description of the setup() step to main simulation docs

90f724b

Removed example_config.toml as I do really see its purpose

428a391

Added details of how to access the example configuration and dummy da…

9cd8139

…ta to usage docs

jacobcook1995 requested review from dalonsoa, davidorme and vgro June 7, 2023 10:12

vgro approved these changes Jun 7, 2023

View reviewed changes

davidorme approved these changes Jun 8, 2023

View reviewed changes

More structure + mermaid diagram in main_simulation doc

694e40b

jacobcook1995 commented Jun 8, 2023

View reviewed changes

davidorme added 2 commits June 8, 2023 13:04

Text tweaks to main_simulation doc

251cf3d

Forgot to add sphinxcontrib-mermaid

6ce98b8

vgro merged commit c60e6cb into develop Jun 14, 2023

vgro deleted the bugfix/vr_run_problems branch June 14, 2023 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various bug fixes to allow `vr_run` to work with `soil` and `abiotic_simple` models#236

Various bug fixes to allow `vr_run` to work with `soil` and `abiotic_simple` models#236
vgro merged 23 commits intodevelopfrom
bugfix/vr_run_problems

jacobcook1995 commented Jun 7, 2023

Uh oh!

jacobcook1995 commented Jun 7, 2023

Uh oh!

codecov-commenter commented Jun 7, 2023 •

edited

Loading

Uh oh!

vgro left a comment

Uh oh!

davidorme left a comment

Uh oh!

davidorme Jun 8, 2023

Uh oh!

davidorme Jun 8, 2023

Uh oh!

jacobcook1995 Jun 8, 2023

Uh oh!

davidorme Jun 8, 2023

Uh oh!

jacobcook1995 Jun 8, 2023

Uh oh!

davidorme Jun 8, 2023

Uh oh!

vgro commented Jun 13, 2023

Uh oh!

davidorme commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jacobcook1995 commented Jun 7, 2023

Description

Type of change

Key checklist

Further checks

Uh oh!

jacobcook1995 commented Jun 7, 2023

Uh oh!

codecov-commenter commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vgro left a comment

Choose a reason for hiding this comment

Uh oh!

davidorme left a comment

Choose a reason for hiding this comment

Uh oh!

davidorme Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

davidorme Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

jacobcook1995 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

davidorme Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

jacobcook1995 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

davidorme Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

vgro commented Jun 13, 2023

Uh oh!

davidorme commented Jun 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jun 7, 2023 •

edited

Loading