-
Notifications
You must be signed in to change notification settings - Fork 5
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
Revise API for component parameters, constants, and states #39
Merged
ThibHlln
merged 26 commits into
unifhy-org:dev
from
ThibHlln:revise-parameters-constants
May 6, 2021
Merged
Revise API for component parameters, constants, and states #39
ThibHlln
merged 26 commits into
unifhy-org:dev
from
ThibHlln:revise-parameters-constants
May 6, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These can contain e.g. number of soil layers, or effective soil depth. These can be useful to initialise different different soil layers, or to initialise soil layers at a given height related to effective soil depth. These are only examples. Given these does not cost much as initialise and finalise are called only once per cycle/run, and these methods should all feature a **kwargs to catch all arguments not caught in the component's method signature.
The default value used to be set in the component's run method signature. This meant that the value was only easily known in this method only (ruling out using it in initialise or finalise methods for instance). Also, keeping the constant definition and its default value so "far apart" was probably increasing the risk of error in assigning a value for a given constant name. Finally, this makes the IRF component methods one step closer to a signature that would be identical in Python/C/Fortran (i.e. setting default values in prototype itself is a Python thing only). Dummy and actual science components are updated accordingly.
This is done by expecting a `cf.Data` in place of a scalar, as was previously the case. Then, the units can easily be checked against those in the component definition. At this stage, the parameters are not converted if the units are "equivalent", but this could be done using `cfunits` in the future, if this becomes a requirement. The YAML configuration file needed to be modified accordingly too. In YAML, a list [value as numeric, unit as string] is expected in place of the numeric value that was expected before. The tests, including the YAML files, are modified accordingly.
These were not used and the tests, and will probably never be used as they do serve a greater purpose than the dummy components as they are.
a sequence of two items (i.e. (value, units) or [value, units]) is now also supported in addition to cf.Data. This sequence is unpacked to create a cf.Data. This means there is no need for the user to explicitly `import cf`, even though cf remains a dependency of `cm4twc`. This also means a more straightforward `Component.from_config()` approach.
…meDomain These tests used to be carried out in methods of Component working on the datasets, but they are useful elsewhere (e.g. in flow_direction and land_sea_mask property setters of SpaceDomain) and will also be re-used later in Component to check for parameters and constants (if provided as fields). /!\ while implementing this, I found out that the `is_space_equal` was missing in flow_direction setter (it was only doing the subset), so it was not checking for the actual spatial metadata compatibility
should have been removed in c9ba49f
The supported types for non-scalar values are: - sequence (array, units) - cf.Data - cf.Field If they are cf.Field, there is a subset performed on the values using the component SpaceDomain, and then a comparison to see if spatial metadata match. If they are cf.Data or sequence (array, units), there can be no spatial metadata match or subset because they are not spatially explicit, so only the shape of the arrays are compared with the shape of the component SpaceDomain. For scalar component values, a corresponding array of the size of the component SpaceDomain is created and filled with the scalar. This way, a component will always know what will be the shape of the parameter it is receiving, which is particularly important for Fortran and C components. The tests are updated to include parameters provided as a cf.Field (in addition to existing cf.Data and sequence [scalar, units]).
not very useful information, especially for parameter now they can be non-scalar, so dropped all three for sake of consistency
related to unifhy-org#31 This partially resolves issue 31 by supporting the use of a constant name (provided as a string) as a valid entry for a state 'divisions' item. If the entry is a string, it will look for this entry in the constant names, and if found, use the value held there. So it will either use the constant 'default_value', or the user-defined overwritten value, thus bringing the customisation looked for in unifhy-org#31.
Since in the dummy tests, only single-division (i.e. scalar) states are used, this was not picked up, but the dimensions in the NetCDF files need to feature one for the state divisions is greater than one. This is now fixed, and multi-division (i.e. vector) states can be recorded.
related to unifhy-org#31 Together with e1b5768, this resolves unifhy-org#31 by adding support for multiple dimensions for component state divisions, e.g. considering some soil layers and some soil types at the same time, one can now use a tuple/list for 'divisions' item. Values in the tuple/list can be numbers and/or strings (if strings, it will look for the value in constants, as implemented in e1b5768).
This should have been done in 59e04c2
Since 4b5e64f, parameters are always given as arrays (even if provided as scalar), this was precisely to make sure a Fortran/C component would know the rank of the array it gets so matter what. But the dummy components were modified accordingly.
When recording component states with (multiple) divisions, the shape of the land_sea_mask used to mask component records was not adjusted accordingly. This is not picked up by the tests because the component that features a land_sea_mask (dummy surfacelayer) does not feature a state with divisions, so when recording its states, the shape of the mask was correct. In dummy openwater, there is a state with divisions but it has no mask, so the mismatch in shape is not picked up.
related to 0585120
related to 0585120 not the whole state array was updated
since they are not used in the basic test suite, only in the advanced test suite
ThibHlln
changed the title
Revise API for component parameters and constants
Revise API for component parameters, constants, and states
May 5, 2021
rich-HJ
approved these changes
May 6, 2021
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.
I think bringing the default value into the component rather than in the runtime call is cleaner and easier to understand.
Having the added flexibility to vary the number of divisions is good additional functionality.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In
Component
definition (relevant for contributors):Old API
New API
In
Component
implementation (relevant for contributors):In
Component
instantiation (relevant for users):cf.Data
with units or using a pair [data, units])cf.Field
, acf.Data
, or a pair [numpy.array
, units]), which resolves Support scalar and array for component parameters #21Old API
New API