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

Initial implementation of CBC solver. #215

Merged
merged 6 commits into from
Jul 7, 2024
Merged

Initial implementation of CBC solver. #215

merged 6 commits into from
Jul 7, 2024

Conversation

jetuk
Copy link
Member

@jetuk jetuk commented Jul 2, 2024

The current CBC C interface does not support modifying constraint coefficients. Therefore this solver can not support aggregated nodes with factors at present. In theory it could support constant factors, but that is also not implemented here.

If this initial version is OK there are two follow-ups to come:

  1. Refactor feat: Initial commit of a mutual exclusivity node. #198 to work with CBC as well.
  2. Use an updated version of CBC that supports constraint coefficient modification.

@jetuk jetuk requested a review from Batch21 July 2, 2024 13:24
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.

Couple of trivial comments but otherwise this looks good to me.

Would it be worth adding the solver as a cli option in this PR? Relatedly, I do not think it is currently possible to run the feature-gated solvers via the cli. Looks like it requires the features to be duplicated in the cli toml file, unless there is a way to have shared features in a workspace?

@@ -0,0 +1,92 @@
use crate::solvers::SolverSettings;

/// Settings for the OpenCL IPM solvers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Solver name needs updating

let coefs = &elements[start as usize..end as usize];

unsafe {
let c_name = CString::new("row").expect("Failed to create CString for column name.");
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 '...for row name."'

@jetuk
Copy link
Member Author

jetuk commented Jul 6, 2024

Would it be worth adding the solver as a cli option in this PR? Relatedly, I do not think it is currently possible to run the feature-gated solvers via the cli. Looks like it requires the features to be duplicated in the cli toml file, unless there is a way to have shared features in a workspace?

I'll probably do this when finishing #18. It's an issue in all of the other crates that depend on pywr-core.

I've addressed the comments.

@jetuk jetuk merged commit f954435 into main Jul 7, 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