Refactor parameters into General, Simple and Constant #194
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
GeneralParameter contains compute and after methods, Parameter contains the others.
This is designed as an alternative to #192. The high level idea is the solve the circular parameter issue with initial volumes by recognising that some parameters do not vary in time and/or depend no node state. We introduce the General, Simple and Constant parameters as new traits: - `GeneralParameter` - rename of the current implementation. - `SimpleParameter` - Parameters that only depend on other Simple or Constant parameters, but do vary in time. - `ConstantParameter` - Parameters that only depend on other Constant parameters, and do *not* vary in time. Simple parameters are evaluated before general and `.before` methods each time-step. This allows them to vary in time, but means that can't depend on network or general parameter values. This is most relevant for profiles which have no dependencies but do vary in time. Constant parameters would be evaluated at the beginning of the simulation only. NB, this is not yet implemented. Some parameters can potentially implement one or more of these traits. For example, the AggregatedParameter is implemented for General and Simple depending on whether it is aggregating over MetricF64 or SimpleMetricF64. This allows the user to use an aggregated parameter as a simple parameter provided it is only aggregating over simple parameters. From the schema point of view there is no difference. When added to the core model it attempts to add it as a simplified version if possible.
Batch21
reviewed
Jun 5, 2024
| )); | ||
| } | ||
|
|
||
| match parameter.try_into_simple() { |
There was a problem hiding this comment.
Maybe add a bit of explanation here in a comment? Potentially the function name could be updated to reflect the fact that a simple parameter could be added instead of a general one.
| MultiParameterValue((ParameterIndex<MultiValue>, String)), | ||
| ParameterValue(GeneralParameterIndex<f64>), | ||
| IndexParameterValue(GeneralParameterIndex<usize>), | ||
| MultiParameterValue((GeneralParameterIndex<MultiValue>, String)), |
There was a problem hiding this comment.
Minor point but should these 3 variants be put into a nest enum to be consistent with the simple params?
Final parameter variant that is computed only at the beginning of a model.
This is now fixed. It's ready for a final review. |
Batch21
reviewed
Jul 4, 2024
pywr-core/src/parameters/mod.rs
Outdated
| &mut self, | ||
| parameter: Box<dyn SimpleParameter<usize>>, | ||
| ) -> Result<SimpleParameterIndex<usize>, PywrError> { | ||
| // TODO Fix this check |
There was a problem hiding this comment.
Should this be fixed in this PR?
Batch21
reviewed
Jul 4, 2024
pywr-core/src/parameters/mod.rs
Outdated
| &mut self, | ||
| parameter: Box<dyn SimpleParameter<MultiValue>>, | ||
| ) -> Result<SimpleParameterIndex<MultiValue>, PywrError> { | ||
| // TODO Fix this check |
Implements the same API for all 3 types. Adds checks to ensure names are unique across all parameter variants.
|
This has been merged with master and the above issues addressed. |
Batch21
approved these changes
Jul 5, 2024
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.
This is designed as an alternative to #192.
The high level idea is the solve the circular parameter issue with initial volumes by recognising that some parameters
do not vary in time and/or depend no node state.
We introduce the General, Simple and Constant parameters as new traits:
GeneralParameter- rename of the current implementation.SimpleParameter- Parameters that only depend on other Simple or Constant parameters, but do vary in time.ConstantParameter- Parameters that only depend on other Constant parameters, and do not vary in time.Simple parameters are evaluated before general and
.beforemethods each time-step. This allows them to vary in time, but means that can't depend on network or general parameter values. This is most relevant for profiles which have no dependencies but do vary in time.Constant parameters would be evaluated at the beginning of the simulation only. NB, this is not yet implemented.
Some parameters can potentially implement one or more of these traits. For example, the AggregatedParameter is implemented for General and Simple depending on whether it is aggregating over MetricF64 or SimpleMetricF64. This allows the user to use an aggregated parameter as a simple parameter provided it is only aggregating over simple parameters. From the schema point of view there is no difference. When added to the core model it attempts to add it as a simplified version if possible.
@Batch21 @s-simoncelli @laurenpetch any thoughts on this vs #192 ? Sorry, it's a big refactor / change this one.