Interface SCIP methods - #1237
Conversation
There was a problem hiding this comment.
@Joao-Dionisio were there some changes in the ruff config? I didn't even touch these lines in my commits.
If it's ok I can refactor it to use the more modern X | Y syntax as suggested in the error message
There was a problem hiding this comment.
This is likely just a new ruff version flagging some errors. I'd just merge without caring much about this, but if you're offering to fix this, I'm more than happy to accept :)
I'll take a little bit to review, but everything seems to be in order. Can you please make sure that the optimized_model() manages to find primal solutions?
There was a problem hiding this comment.
I checked that optmized_model() wasn't able to find any primal solution, so I 've increased the node limit parameter and now it finds 1.
Regarding the X | Y syntax, I only changed it in scip.pyi to pass the pipeline checks and because this syntax is already used there.
I was thinking about replacing typing.Union in the entire repo, but in general such fixes are considered unsafe for Python versions prior to 3.10 (see for reference: https://docs.astral.sh/ruff/rules/non-pep604-annotation-union/) and pyproject.toml only requires python>=3.8. Maybe there's no point in touching it until the project moves to python>=3.10?
There was a problem hiding this comment.
Yes, we tend to prefer supporting as many versions as possible, and 3.10 is not thaaat old yet.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1237 +/- ##
==========================================
- Coverage 57.91% 57.29% -0.62%
==========================================
Files 26 27 +1
Lines 5807 5936 +129
==========================================
+ Hits 3363 3401 +38
- Misses 2444 2535 +91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Sorry for the delay @adamj34 ! I'm not as free in September as I was expecting to be, but I'll still manage to take a closer look. I'm not sure what the code coverage bot is complaining about (am AFK), but it'd be nice to ensure that everything is tested. |
Co-authored-by: João Dionísio <57299939+Joao-Dionisio@users.noreply.github.com>
| # very simple model with 2 primal solutions | ||
| # created because getting more than 1 primal solution from random_mip_1 requres setting a large node limit, which slows down the tests | ||
| @pytest.fixture | ||
| def optimized_model_with_primal_solutions(): |
There was a problem hiding this comment.
There's no guarantee that this will remain working for future SCIP versions, but I suppose the same would be true for every model.
There was a problem hiding this comment.
While I'm not sure how likely it was to break in the future, I think it would be better to have something more reliable than assuming that an x number of node limit will produce y number of solutions.
I came up with choosing a simple model and providing solutions by hand, so that it's easy to reason about all possible solutions. I also added a few comments in the tests to make them clearer.
Please let me know if this approach is better.
| upperbound = optimized_model_with_primal_solutions.getUpperbound() | ||
|
|
||
| assert isinstance(first_primal, float) | ||
| assert optimized_model_with_primal_solutions.isGT(first_primal, upperbound) |
There was a problem hiding this comment.
One is in the original space and the other in the transformed space, right?
There was a problem hiding this comment.
Yes, getUpperbound() is in the transformed space and getFirstPrimalBound() is in the original space. However, I've refactored this test a bit and no longer use getUpperbound().
| leaves, children, siblings = optimized_model.getOpenNodes() | ||
| open_nodes = leaves + children + siblings | ||
| manual_avg_lowerbound = 0.0 | ||
| if len(open_nodes) > 0: |
There was a problem hiding this comment.
This is 0 if the model is solved to optimality, so I suppose we should assert that it isn't.
There was a problem hiding this comment.
I deleted the if statement.
|
|
||
|
|
||
| def test_getAvgDualbound(optimized_model): | ||
| avg_dualbound = optimized_model.getAvgDualbound() |
There was a problem hiding this comment.
This has the same original vs transformed space fragility. But, I suppose it's not a big deal, it's a minimization problem, and any SCIP changes to the way it handles this will error out very loudly everywhere.
There was a problem hiding this comment.
I think that for a minimization problem getAvgDualbound() and getAvgLowerbound() will be exactly the same.
For a maximization problem, one will be the negative of the other. That's why I included or in the assert statement.
There was a problem hiding this comment.
We could test it similarly to test_getAvgLowerbound but there's no getDualbound() method defined on Node. This is the reason behind using a comparison to getAvgLowerbound for testing method.
|
Thank you for your suggestions and comments @Joao-Dionisio! I believe I've addressed all of them. If there's anything that requires further improvement, then let me know. Regarding the Codecov complaints, I double checked if all methods that I added were tested. It seems that PS I hope that these reviews aren't taking too much time away from your PhD 😊 |
Adds the following methods:
getNNodesLeft(),getNRuns(),getNReoptRuns(),addNNodes(),getDeterministicTime(),getAvgDualbound(),getMaxTotalDepth(),getNBacktracks(),getFocusNode(),getAvgLowerbound(),getFirstPrimalBound(),getLowerboundRoot(),getUpperbound(),getNObjlimLeaves()Adds tests in
tests/test_statistics.pyandtests/test_node.pyfor migrated methodsAdds function signatures to
scip.pyifor type hintsUpdates
CHANGELOG.md