-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more functionality into LayerStructure. #452
Add more functionality into LayerStructure. #452
Conversation
@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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I review the wrong PR despite @davidorme being explicit about which one I should review...
Anyway, these changes make total sense if aggregate roles are really a thing. Nevertheless, there are a couple of suggestions in the other PR that you might want to consider.
@dalonsoa Aggregate roles are very much a thing but we could just try and define all of them as named roles (like |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much cleaner 👍
a2d19e9
into
458-adopt-extended-layerstructure-functionality
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.__post_init__
method has been modularised to make it easier to follow.role_indices
dictionary attribute has switched over to using properties for a cleaner and more compact API. The underlying_role_indices_bool
and_role_indices_int
remain but haveindex_role_name
properties to access role indices.filled_canopy
,filled_atmosphere
andflux_layers
roles have been added.set_filled_canopy
method takes an array of canopy heights and updates those roles as well as populating thelowest_canopy_filled
attribute for each grid cell.canopy_layers
->n_canopy_layers
etc).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
pre-commit
checks:$ pre-commit run -a
$ poetry run pytest
Further checks