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

feat: Initial commit of a mutual exclusivity node. #198

Merged
merged 9 commits into from
Sep 23, 2024

Conversation

jetuk
Copy link
Member

@jetuk jetuk commented Jun 7, 2024

This is the first commit of using the Highs solver to model binary variables, and using those variables to apply a mutual exclusivity constraint. I.e. flow is allow through up to N nodes at a time.

TODO:

  • Add support in pywr-schema.
  • Implement in CLP/CBC solver.
  • Add additional tests.

This starts to resolve issue #187.

@jetuk
Copy link
Member Author

jetuk commented Jun 7, 2024

Re CLP/CBC.

CLP is linear programme only. CBC is the MILP solver that uses CLP internally. We have two choices:

  1. Retain the CLP solver for LP only models, and add a CBC solver for LP & MILP models.
  2. Replace the CLP with a CBC solver.

I suggest (2) is preferrable as long as CBC isn't significantly slower for the LP only case.

It is probably worthwhile doing this switch in another issue/PR before completing this one.

@jetuk jetuk force-pushed the issue187-add-binary-flows branch from 00c1481 to 03bd888 Compare June 7, 2024 21:11
@jetuk
Copy link
Member Author

jetuk commented Jun 7, 2024

Also, the tests will fail on the lack of support in CLP first.

@jetuk
Copy link
Member Author

jetuk commented Jun 12, 2024

I've created a project to try to organise how to move this forward: https://github.com/orgs/pywr/projects/1

@jetuk jetuk force-pushed the issue187-add-binary-flows branch from 03bd888 to e9c057c Compare July 9, 2024 15:43
jetuk added 3 commits July 30, 2024 20:33
This is the first commit of using the Highs solver to model
binary variables, and using those variables to apply a mutual
exclusivity constraint. I.e. flow is allow through up to N nodes
at a time.

TODO:
- [ ] Add support in pywr-schema.
- [ ] Implement in CLP/CBC solver.
- [ ] Add additional tests.

This starts to resolve issue #187.
All tests pass as expected with solver features.
A refactor of the aggregated node implementation to allow
support for sets of nodes. These are typically created when a
compound node (e.g. PiecewiseLink) is added to an AggregatedNode.
The compound node creates multiple "core" nodes, and these should
all be constrained by the AggregatedNode's factors or exclusivity.

The same consideration applies to the virtual storage nodes.

To do this there is a new method on Node in the schema to retrieve
the core node indices which that node has added to the core model.
For now this is a static implementation, but there are some nodes
which the user may to configure how this is done. E.g. should a
LossLink be constrained on its net or gross flow.
@jetuk jetuk force-pushed the issue187-add-binary-flows branch from e9c057c to 9825b96 Compare August 2, 2024 14:24
@jetuk jetuk marked this pull request as ready for review August 2, 2024 14:25
Copy link
Contributor

@Batch21 Batch21 left a comment

Choose a reason for hiding this comment

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

a few queries but this looks good to me. I probably need to spend a bit more time exploring the solver code.

Ok(indices)
}
pub fn add_to_model(&self, network: &mut pywr_core::network::Network, args: &LoadArgs) -> Result<(), SchemaError> {
let nodes: Vec<Vec<_>> = self
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it duplicates some of the node_indicies_for_constraints func above. Could that not be used here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's slightly different. This version is a nested vec. Each entry of the outer vec is a vec of that node's indices (i.e. it could refer to multiple core nodes). The above function is a flattened version of this array.

@@ -676,6 +711,7 @@ impl TryFrom<CatchmentNodeV1> for CatchmentNode {
pub enum Factors {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be worth changing the name of this enum to Relationship to match core? the name of the agg node attribute could also be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -676,6 +711,7 @@ impl TryFrom<CatchmentNodeV1> for CatchmentNode {
pub enum Factors {
Proportion { factors: Vec<Metric> },
Ratio { factors: Vec<Metric> },
Exclusive,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you going to add the ability to set exclusivity parameters (e.g. min/max active) in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

"mutual-exclusivity-12",
None,
&[vec![link_node1], vec![link_node2]],
Some(Relationship::new_exclusive(0, 1)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a test for the min_active constraint?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this might be quite tricky without minimum flow support. It could activate something, but then not use it. However, it does beg the question whether we should be able to output the state of the binary variables.

self.builder.coefficients_to_update.push((row.row_id, bin_col_id, -ub));

// This row is upper bounded to zero
self.builder.apply_row_bounds(row.row_id.to_usize().unwrap(), FMIN, 0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

are these bounds not already set in create_node_constraints and applied by the builder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they are.

@jetuk jetuk requested a review from Batch21 September 21, 2024 12:54
@jetuk jetuk merged commit aeba898 into main Sep 23, 2024
7 checks passed
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.

2 participants