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: Add support for multi-model simulation. #60

Merged
merged 16 commits into from
Jan 12, 2024
Merged

feat: Add support for multi-model simulation. #60

merged 16 commits into from
Jan 12, 2024

Conversation

jetuk
Copy link
Member

@jetuk jetuk commented Oct 19, 2023

Add PywrMultiModel schema and corresponding MultiNetworkModel to pywr-core. This update refactors the schema of the regular model to use a new "network" key. The schema then provides a PywrNetwork containing only the nodes, edges, parameters, metric sets and outputs. This is then reused by the regular model and the multi-model.

A new parameter is added that can receive a parameter from another model. This is implemented via the transfers in the multi-model configuration. This is fairly generic because it uses the metric system to allowing passing of node or parameter data from one model to another.

TODO:

  • Write some documentation on the new structs.
  • Check naming convention for inter-model transfers.
  • Add a multi-model test that uses circular inter-model transfers.

Add `PywrMultiModel` schema and corresponding `MultiNetworkModel` to pywr-core. This update refactors the schema of the regular model to use a new "network" key. The schema then provides a `PywrNetwork` containing only the nodes, edges, parameters, metric sets and outputs. This is then reused by the regular model and the multi-model.

A new parameter is added that can receive a parameter from another model. This is implemented via the transfers in the multi-model configuration. This is fairly generic because it uses the metric system to allowing passing of node or parameter data from one model to another.

TODO:

- [ ] Write some documentation on the new structs.
- [ ] Check naming convention for inter-model transfers.
- [ ] Add a multi-model test that uses circular inter-model transfers.
@jetuk jetuk requested a review from Batch21 October 19, 2023 17:53
@jetuk jetuk changed the title feat: Add support for multi-model simulation. [DRAFT] feat: Add support for multi-model simulation. Oct 19, 2023
@jetuk jetuk marked this pull request as draft October 20, 2023 09:34
@jetuk jetuk changed the title [DRAFT] feat: Add support for multi-model simulation. feat: Add support for multi-model simulation. Oct 20, 2023
@jetuk jetuk marked this pull request as ready for review October 24, 2023 12:57
@jetuk jetuk mentioned this pull request Oct 24, 2023
72 tasks
jetuk added 5 commits November 9, 2023 21:52
Remove the need for placeholder parameters to receive inter-model
transfer values. Instead a metric is used to indicate the name
of the inter-model transfer value to use at that particular
location. This simplifies the configuration of inter-model
transfers, but requires the build methods in the schema to recieve
a reference to any transfers.
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.

Latest changes look good and all the tests/models run succesfully for me. Made a few suggestions but happy if you want to get this merged before making additional changes.

/// A Pywr model containing multiple link networks.
///
/// This schema is used to define a model containing multiple linked networks. Each network
/// is self-contained and solve as like a single a model. However, the networks can be linked
Copy link
Contributor

Choose a reason for hiding this comment

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

should be 'solved as a single model'?

/// Calculate inter-model parameters for the given scenario index.
///
///
fn compute_inter_network_transfers(
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a unit test for this func that test a multi-model with 3 or more networks

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 I'll add this as an issue. I have a plan for a longer example that would require passing index (integer) values between models as well.

@@ -173,48 +172,36 @@ fn run(path: &Path, solver: &Solver, data_path: Option<&Path>, output_path: Opti
let data = std::fs::read_to_string(path).unwrap();
let schema_v2: PywrModel = serde_json::from_str(data.as_str()).unwrap();

let (model, timestepper): (Model, Timestepper) = schema_v2.build_model(data_path, output_path).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a cmd needs adding to run a multi-model. Something woth doing in this PR?

parallel,
threads,
debug,
} => run(model, solver, data_path.as_deref(), output_path.as_deref(), *debug),
Copy link
Contributor

Choose a reason for hiding this comment

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

should be run_multi?

@jetuk jetuk merged commit 651d7bb into main Jan 12, 2024
2 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