Closed davidorme closed 2 months ago
First review of WIP
@jacobcook1995 and @vgro - Do you need any of this implementation changing or extending? @alexdewar Anything obviously mad?
Yeah looks sensible to me, I'm not planning to continue using topsoil_index
in the long run (I don't know if @vgro is though), so from my perspective I'm happy with any implementation that doesn't break the current tests
Looks sensible to me, also good job tidying up the conftest :-) One concern is that I have the parallel branch for including stats which and I don't know in which order we want to merge this in develop to make the least mess ?
This PR is a little on the long side, as @vgro says. Not sure how easy it would have been to break this work up, but in general I think little PRs often is better than big PRs occasionally, especially when the work touches a lot of different areas of code.
@alexdewar. I completely agree - the problem here is that one of the core changes we want to make in this PR changes the vertical structure of the simulation so that the dimensions of the data change. That's not something that we can adopt incrementally, but we are trying to isolate the bits for different models in sub-branches. Thanks for the comments!
@dalonsoa Any idea on the qa
failure here? The pre-commit.ci
is running the same thing with the same configuration - wait. Might be python version?
The only other thing I looked at is the mypy
configuration in pyproject.toml
specifies the numpy.testing.mypy-plugin
, but that shouldn't affect the pre-commit
configuration and that is identical between the two actions.
Not obviously a python version thing. I've switch my poetry
environment between 3.10, 3.11 and 3.12 and pre-commit run -a
passes on all three.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 94.75%. Comparing base (
d7298af
) to head (778ba2b
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@vgro Can you approve this - we know it's a partial implementation of what is coming in #452 but we should bank it and then we can work on implementing #452 on a separate PR.
@dalonsoa I think we need you to approve this too after your requested changes. We know it's a halfway house, but we want to be able to move on with other stuff in develop
while the changes in #452 are adopted.
Description
This is a work in progress PR implementing the removal of the explicit subcanopy layer from the vertical layer structure.
The current progress is
subcanopy
layer. Thelayer_roles
attribute is now shorter by 1.LayerStructure.from_template(name)
method is added, returning a fresh blank named DataArray withlayer_roles
andcell_id
dimensions.LayerStructure.role_indices
andLayerStructure.role_indices_bool
, depending on whether you want integer indices or boolean indices onto the layer role dimension. Those are keyed byabove
,canopy
,surface
,soil
.topsoil
indexing option - the soil layer roles are now explicitly split into a singletopsoil
role and then additionalsubsoil
layers but an additionalall_soil
index is provided to refer to all soil layers collectively.New location for Grid
[X] As a side effect, the
Grid
object now also exists withinCoreComponents
and is inherited by models automatically asModel.grid
via theBaseModel.__init__
mechanism. As noted in #376, this is saner place to put it now thatCoreComponents
exists, and theLayerStructure
component needs some minimal information from the grid.This PR will not remove
Data.grid
so we will have two copies of that for a while, but this will be the long term home and it is easier to work with two copies than try and do this all in one go. At some point a separate PR will fix the other half of #376 and remove the dupe fromData
.Things to do:
Fix everything that this breaks:
These fixes can be by individual PR onto this branch to pick off issues until we have a passing PR to merge down.
Fixes #421 (issue)
Type of change
Key checklist
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks