Skip to content

Add add_cons_local, add_cons_node #230

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

Merged
merged 20 commits into from
Apr 3, 2025
Merged

Add add_cons_local, add_cons_node #230

merged 20 commits into from
Apr 3, 2025

Conversation

Joao-Dionisio
Copy link
Member

Still not done, want to take my time to make things properly, as in adding tests and actually understanding what chatGPT combined with Rust's compiler have created. Ah, and probably want to create the constraint directly on the function, and also want to add add_cons_node.

@mmghannam a question about the russcip philosophy. Is it like PySCIPOpt where you want to mimic SCIP as much as possible?

@mmghannam
Copy link
Member

@Joao-Dionisio happy to see you here :)

Unlike PySCIPOpt one could here call the SCIP api directly from the ffi module. So russcip is more free in what it can offer. The philosophy is mainly two things:

  1. Make the default use case easy.
  2. Try to avoid as many user errors as possible at compile time.

@mmghannam
Copy link
Member

I think in this case an add_cons method on the node struct (that takes a ConsBuilder object) would be more ergonomic.

@Joao-Dionisio
Copy link
Member Author

Joao-Dionisio commented Mar 28, 2025

I think in this case an add_cons method on the node struct (that takes a ConsBuilder object) would be more ergonomic.

Same, that's what made me question russcip's philosophy :) Just need to grab the local model from the node, but it should be okay

EDIT: on the other hand, I'm not seeing how to access the local scip

EDIT2: Should be mostly done. Just need to add a test, which will be a bit annoying because of the branching requirement... unless it's enough to test if adding at the root node is enough 🤔

@Joao-Dionisio Joao-Dionisio changed the title Add add_cons_local Add add_cons_local, add_cons_node Mar 28, 2025
@mmghannam
Copy link
Member

mmghannam commented Mar 28, 2025

I think these need to be only on Model<Solving>. I am not sure if it makes sense at any other stage. Then, you could modify the test in the separator.rs to addConsLocal instead, and you can modify the custom branching rule test to additionally use addConsNode.

@Joao-Dionisio Joao-Dionisio requested a review from mmghannam March 28, 2025 14:37
@Joao-Dionisio Joao-Dionisio marked this pull request as ready for review March 28, 2025 14:37
@mmghannam mmghannam enabled auto-merge April 3, 2025 12:12
@mmghannam mmghannam merged commit b6a4260 into main Apr 3, 2025
7 checks passed
@mmghannam mmghannam deleted the add_cons_local branch April 3, 2025 12:14
@Joao-Dionisio
Copy link
Member Author

Wait @mmghannam , did you manage to fix the underlying problem?

Why wasn't node_get_n_added_cons not working? I'm assuming it was due to SCIP somehow treating the current node differently, but I couldn't figure out what was wrong.

@mmghannam
Copy link
Member

Wait @mmghannam , did you manage to fix the underlying problem?

Why wasn't node_get_n_added_cons not working? I'm assuming it was due to SCIP somehow treating the current node differently, but I couldn't figure out what was wrong.

The function was working with the use of add_cons_node() in the branching rule tests. I think add_cons_local doesn't have the same effect on the current node, it might be upgraded to a global constraint since it was running in the root node but I'm not sure.

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