Skip to content
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 soft constraints in link node #213

Merged
merged 13 commits into from
Oct 1, 2024

Conversation

s-simoncelli
Copy link
Contributor

No description provided.

@s-simoncelli s-simoncelli marked this pull request as draft June 26, 2024 14:01
@s-simoncelli
Copy link
Contributor Author

If there are no problems with the implementation, I may start writing a paragraph about this in the book

@s-simoncelli s-simoncelli marked this pull request as ready for review June 26, 2024 14:30
@jetuk
Copy link
Member

jetuk commented Jun 26, 2024

I've only had quick read through this. My initial reaction is whether the main min_flow and max_flow values should always be hard constraints. And that a sensible configuration is for the soft flow bounds to be inside the range of min_flow and max_flow. I.e. the soft max should be lower than the hard max.

I think this would require an aggregated node to enforce the hard min & max flows across the multiple internal nodes if either soft bound is defined, and the soft max applied to the main L node.

@Batch21 thoughts?

@Batch21
Copy link
Contributor

Batch21 commented Jun 27, 2024

Keeping the soft constraints within the bounds of min_flow and max_flow makes sense to me.

Could apportioning volumes to the different links be an alternative to using an agg node. Something like this:

L_min max_flow = soft_min
L_max max_flow = max_flow - soft_max
L max_flow = max_flow - soft_min - (max_flow - soft_max)

@s-simoncelli
Copy link
Contributor Author

Apologies for not reply to this sooner but it's been a busy month. Wouldn't it make more sense to have the node L downstream to enforce the hard min and max flows?

image

We can then add 2 aggregated nodes:

  • Constraining L and L_min with the min_flow of node L
  • Constraining L and L_max with the max_flow of node L

Altought one can still set the min_flow of L_min larger than the max_flow of L_max

@jetuk
Copy link
Member

jetuk commented Aug 6, 2024

We can then add 2 aggregated nodes:

* Constraining  `L` and `L_min` with the `min_flow` of node `L`
* Constraining  `L` and `L_max` with the `max_flow` of node `L`

I'm not sure I follow this. This constraint would be redundant because it could just be applied to L and achieve the same thing?

I was thinking of it like this:

image

If soft min and max defined:

  • a: min_flow = 0.0, max_flow = unconstrained, cost = user's soft_max_flow_cost (i.e. cost of going above soft max)

  • b: min_flow = 0.0, max_flow = unconstrained, cost = user's standard cost

  • c: min_flow = 0.0, max_flow = user's soft_min_flow, cost = user's soft_min_flow_cost

  • agg_bc: nodes: [b, c], min_flow = 0.0, max_flow = user's soft_max_flow

  • agg_abc: nodes: [a, b, c], min_flow = user's hard min_flow, max_flow = user's hard_max_flow

if only soft min defined:

  • a: not created

  • b: min_flow = 0.0, max_flow = unconstrained, cost = user's standard cost

  • c: min_flow = 0.0, max_flow = user's soft_min_flow, cost = user's soft_min_flow_cost

  • agg_bc: nodes: [b, c], min_flow = user's hard min_flow, max_flow = user's hard_max_flow

If only soft max defined:

  • a: min_flow = 0.0, max_flow = unconstrained, cost = user's soft_max_flow_cost (i.e. cost of going above soft max)

  • b: min_flow = 0.0, max_flow = user's soft_max_flow, cost = user's standard cost

  • c: not created

  • agg_abc: nodes: [a, b], min_flow = user's hard min_flow, max_flow = user's hard_max_flow

If neither soft max or soft min defined:

  • a: not created
  • b: min_flow = user's hard min_flow, max_flow = user's hard_max_flow, cost = user's standard cost
  • c: not created

@s-simoncelli
Copy link
Contributor Author

Hello,

I implemented the new constraints as suggested. I think it would be nice to export the internal nodes' flow data using metrics. What do you think?

I have no idea why the pipelines fail. On my machine the project compiles

Copy link
Member

@jetuk jetuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I've just noticed a possibly outdated sentence in the docstring. One other very minor comment. At some point we should provide expected output data for the test models.

Happy to merge when the doc string is fixed.

pywr-schema/src/nodes/core.rs Outdated Show resolved Hide resolved
pywr-schema/src/nodes/core.rs Outdated Show resolved Hide resolved
pywr-schema/src/nodes/core.rs Show resolved Hide resolved
@jetuk
Copy link
Member

jetuk commented Sep 30, 2024

I think it would be nice to export the internal nodes' flow data using metrics. What do you think?

Agreed. What are you thinking? Some attributes that might be useful (first two more than the last two):

  • (In|Out)flowAboveSoftMax or SoftMaxFlowExceedance - self explanatory
  • (In|Out)flowBelowSoftMin or SoftMinFlowDeficit - A deficit to the soft min flow.
  • (In|Out)flowAboveSoftMin - flow less the soft min flow
  • (In|Out)flowBelowSoftMax - flow less the soft max flow

WTW node could also implement the soft min related ones.

@s-simoncelli
Copy link
Contributor Author

Hi, thanks for the feedback.

I created two separate issues to address the additional metrics and the test implementation

@jetuk jetuk merged commit ffa28b9 into pywr:main Oct 1, 2024
5 checks passed
@jetuk
Copy link
Member

jetuk commented Oct 1, 2024

Thanks for implementing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants