Turbine node and parameter

[s-simoncelli]

I moved the implementation of the turbine components in a new branch because both the parameter and node have the same dependencies.

• The HydropowerTargetParameter is now initialised with a dedicated struct to reduce the number of inputs to new()
• The TurbineNode accepts target which sets the max_flow using the inverse hydropower equation. Do we want to let the user set the max_flow and min_flow fields if the target is not set? These are private now.
• I need to add a boolean field to let the user set the target as min_flow instead of max_flow.
• The TurbineNode::create_metric() function is still a WIP. The derived metric uses TurbineData to get the data it needs to calculate the power. However, because water_elevation (L163) is a DynamicFloatValue, I need to load it as Metric but I don't have access to the schema, domain, tables, etc. Have I implemented it correctly or does the create_metric signature needs to be changed?

[jetuk]

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 dedicated struct to reduce the number of inputs to new()

Sounds like a good idea.

The TurbineNode accepts target which sets the max_flow using the inverse hydropower equation. Do we want to let the user set the max_flow and min_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 as min_flow instead of max_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 uses TurbineData to get the data it needs to calculate the power. However, because water_elevation (L163) is a DynamicFloatValue, I need to load it as Metric but I don't have access to the schema, domain, tables, etc. Have I implemented it correctly or does the create_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.

[jetuk]

@s-simoncelli are you still working on this as a draft or do you want me to review it?

[s-simoncelli]

It’s ready except for the power metric calculation. You can review it :)

[jetuk]

@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.

[s-simoncelli]

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.

[jetuk]

I've merged main into here again. It was a bit fiddly but is working.

There's a couple of improvements to make before merge.

It would be nice to have a test model with the turbine node / parameter included to verify some of the values.

[jetuk]

I would simplify this function to just accept head: f64. It's only use already calculates the head value and sets turbine_elevation to zero.

[jetuk]

The same for this function.

[jetuk]

        // Bound the flow if required
        if let Some(max_flow) = &self.max_flow {
            q = q.min(max_flow.get_value(model, state)?);
        } 
        if let Some(min_flow) = &self.min_flow {
            q = q.max(min_flow.get_value(model, state)?);
        }

I think this should be two independent if blocks, not an if-else-if.

[jetuk]

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?

[Batch21]

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