ImperialCollegeLondon / virtual_ecosystem

This repository is the home for the codebase for the Virtual Ecosystem project.
https://virtual-ecosystem.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

Add more functionality into LayerStructure. #452

Closed davidorme closed 2 months ago

davidorme commented 2 months ago

Description

[ETA] This PR has been through a couple of incarnations - this description has been updated to reflect the final state of the PR.

This PR implements the updates to the LayerStructure object described in #451 and a few more things.

This will break the current use of layer_structure.role_indices in #421 but that should be a fairly straight swap and results in a much clearer interface: the code explicitly states what roles are needed.

Fixes #451

Type of change

Key checklist

Further checks

davidorme commented 2 months ago

@alexdewar A bit difficult to review this stuff because it's all such a big jigsaw of things being assembled, but all of the changes we're working on are based around the upgraded LayerStructure class in virtual_ecosystem.core.core_components. That is a centralised tool, shared among all models for communicating and coordinating information about the vertical layering of the model.

It's been a bit of a two stage process - the class was updated and then we've identified how best to build out the class further in rolling out those updates. I think this is where the functionality stops for now, though, and since its so central, it would be really useful to get a focussed review of that element as part of this PR.

davidorme commented 2 months ago

@dalonsoa Aggregate roles are very much a thing but we could just try and define all of them as named roles (like all_soil). I just wanted to avoid future models having to tinker with LayerStructure to work. I did consider adding a requires_aggregate_index attribute to the model to set them all up at the start, but didn't want to further complicate the base model structure.

dalonsoa commented 2 months ago

@dalonsoa Aggregate roles are very much a thing but we could just try and define all of them as named roles (like all_soil). I just wanted to avoid future models having to tinker with LayerStructure to work. I did consider adding a requires_aggregate_index attribute to the model to set them all up at the start, but didn't want to further complicate the base model structure.

No, I think this is the right place, and the way you have implemented it is very neat and flexible. I've just added a small suggestion, but totally optional.