Closed s-simoncelli closed 2 months ago
I've not thorough read this yet. Is it still draft, or is it ready for review?
I moved the implementation of the turbine components in a new branch because both the parameter and node have the same dependencies.
Great!
The
HydropowerTargetParameter
is now initialised with a dedicatedstruct
to reduce the number of inputs tonew()
Sounds like a good idea.
The
TurbineNode
acceptstarget
which sets themax_flow
using the inverse hydropower equation. Do we want to let the user set themax_flow
andmin_flow
fields if the target is not set? These are private now.
What happens if the user specifies "target" and "max_flow" in the schema? Is this allowed? Does it take the minimum? Same for a "min_flow". I would say these fields should be removed from the schema if they are not used.
I need to add a boolean field to let the user set the
target
asmin_flow
instead ofmax_flow
.
Another option would be an enum (I'm not sure about the names):
enum TargetType {
MaxFlow // Applies target as a max_flow
MinFlow // Applies target as a min_flow
Fixed // Applies target as both (like a catchment)
}
The
TurbineNode::create_metric()
function is still a WIP. The derived metric usesTurbineData
to get the data it needs to calculate the power. However, becausewater_elevation
(L163) is aDynamicFloatValue
, I need to load it asMetric
but I don't have access to theschema
,domain
,tables
, etc. Have I implemented it correctly or does thecreate_metric
signature needs to be changed?
I'll take a look at this. I was expecting some complication here as this is a not quite like anything currently implemented.
@s-simoncelli are you still working on this as a draft or do you want me to review it?
It’s ready except for the power metric calculation. You can review it :)
@s-simoncelli I've had a look at this. I think it is good. In order to solve the issue in the create_metric
method I've wanted to refactor a few things first. Most of those are done now (e.g. #148 ). The only other one is simplifying metrics in the schema. I am going to work on that next. Once done we can fix this branch and see if it fits together better.
I merged the new changes from the main branche and I pass now &LoadArgs
to the metric functions so that I can use it in the turbuine node.
I've applied the changes I suggestion as they were only minor. I'm tempted to merge this now, and fix-up any issues, examples, etc later. @Batch21 do you agree?
I've applied the changes I suggestion as they were only minor. I'm tempted to merge this now, and fix-up any issues, examples, etc later. @Batch21 do you agree?
yeah, I agree
I moved the implementation of the turbine components in a new branch because both the parameter and node have the same dependencies.
HydropowerTargetParameter
is now initialised with a dedicatedstruct
to reduce the number of inputs tonew()
TurbineNode
acceptstarget
which sets themax_flow
using the inverse hydropower equation. Do we want to let the user set themax_flow
andmin_flow
fields if the target is not set? These are private now.target
asmin_flow
instead ofmax_flow
.TurbineNode::create_metric()
function is still a WIP. The derived metric usesTurbineData
to get the data it needs to calculate the power. However, becausewater_elevation
(L163) is aDynamicFloatValue
, I need to load it asMetric
but I don't have access to theschema
,domain
,tables
, etc. Have I implemented it correctly or does thecreate_metric
signature needs to be changed?