Skip to content

Conversation

@fvz185
Copy link

@fvz185 fvz185 commented Oct 4, 2025

To clarify and document the usage of PySCIPOpt's presolver plugin, a working example and a tutorial were added. They required wrappers to be implemented, too. The wrapper functions are supplemented with tests. The example and tutorial replicate the Boundshift presolver.

However, some open issues remain:

  1. The test for the var.isActive() wrapper is incomplete since it does not cover all possible settings. Should tests on inactive variables due to probing or aggregation be included?
  2. Only SCIP functions that are not easily emulatable within Python and required for the example were wrapped. Should any additional SCIP functions be wrapped to make implementations of custom presolvers more straightforward? This would reduce the efforts needed from PySCIPOpt users to inspect SCIP's source code.

@codecov
Copy link

codecov bot commented Oct 4, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.70%. Comparing base (6452ea6) to head (5ce2777).
⚠️ Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
src/pyscipopt/scip.pxi 75.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
+ Coverage   54.13%   54.70%   +0.56%     
==========================================
  Files          22       24       +2     
  Lines        5045     5327     +282     
==========================================
+ Hits         2731     2914     +183     
- Misses       2314     2413      +99     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mmghannam mmghannam self-assigned this Oct 4, 2025
@Joao-Dionisio Joao-Dionisio requested a review from Copilot October 4, 2025 23:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends PySCIPOpt to support custom presolver plugins by adding wrapper functions, tests, a working example, and documentation. The key enhancement enables users to implement custom presolvers in Python by subclassing the Presol class.

  • Added isActive() method for variables to check if they are active (not fixed or aggregated)
  • Added aggregateVars() method to aggregate two variables with custom coefficients
  • Created a complete Shiftbound presolver example that replicates SCIP's boundshift.c logic

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/pyscipopt/scip.pxi Added isActive() and aggregateVars() wrapper methods for variables and models
src/pyscipopt/scip.pxd Added C function declarations for SCIPvarIsActive and SCIPaggregateVars
tests/test_vars.py Added basic test for isActive() method on newly created variables
tests/test_aggregate_vars.py Added comprehensive tests for aggregateVars() functionality
examples/finished/shiftbound.py Complete presolver example implementing boundshift logic
docs/tutorials/presolver.rst Tutorial documentation explaining presolver plugin usage
CHANGELOG.md Updated changelog with new features

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Member

@mmghannam mmghannam left a comment

Choose a reason for hiding this comment

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

Thank you very much @fvz185 for the useful contribution! I believe this will make it much easier for people to prototype presolving techniques in python.
I've added a few small comments otherwise looks good to merge to me :)

@mmghannam
Copy link
Member

To also answer your questions:

  1. The test for the var.isActive() wrapper is incomplete since it does not cover all possible settings. Should tests on inactive variables due to probing or aggregation be included?

I'm happy with the current test, but if you have the time yes, assertions in your presolver test on the aggregated variables would be nice.

  1. Only SCIP functions that are not easily emulatable within Python and required for the example were wrapped. Should any additional SCIP functions be wrapped to make implementations of custom presolvers more straightforward? This would reduce the efforts needed from PySCIPOpt users to inspect SCIP's source code.

I don't have a lot of experience with presolvers but I would say one should wrap the methods when a use-case/feature request pops up. If you see anything that could be useful to "preemptively" wrap I'd be happy to review it in future PRs :D

@fvz185
Copy link
Author

fvz185 commented Oct 7, 2025

I just realised that, while I was working on the documentation, the implementation of presol_boundshift.c had changed. Besides slight modifications to the sequence, it now no longer uses SCIPvarIsActive(). I'll check and align my documentation again but prioritise to make the presolver documentation/example self-contained/-explanatory.

I'll also take a look again on the tests to capture your suggestions.

Lastly, I think I will crawl through the other presolvers to see which SCIP functions are most frequently used and then wrap them (as part of future PRs). My goal is anyway to better grasp SCIP's source code, so it'll be a good exercise. Just let me know about any other feature that I could take a look at (ofc, I'll also take a look at the currently open issues).

Thank you @mmghannam for your feedback :)

@Joao-Dionisio
Copy link
Member

Hey @fvz185 ! THank you so much for your contribution! We would love to include this in the next release, PySCIPOpt 7.0.0, which coincides with SCIP 10 (should be a couple more weeks, end of the month, start of next month). How are the TODOs looking, is there any way we can help out?

@fvz185
Copy link
Author

fvz185 commented Nov 19, 2025

Hej @Joao-Dionisio :) Got some other priorities to focus on for the moment. You are of course welcome to make and commit any changes that you think you can work on. However, I'll try to free some time within the next two weeks so that these changes here can contribute to PySCIPOpt's upcoming release :)

@Joao-Dionisio
Copy link
Member

@fvz185 I added some of the requested changes. Can you please add permissions for me to push to your branch, so that we can merge this?

@fvz185
Copy link
Author

fvz185 commented Jan 20, 2026

Sure thing! I've added you. Sorry that I'm slacking.

@Joao-Dionisio
Copy link
Member

Thank you!
Ah, don't worry about it, I've only been able to progress because I've been slacking in other areas myself 😀

@Joao-Dionisio Joao-Dionisio self-requested a review January 20, 2026 14:09
@Joao-Dionisio
Copy link
Member

@mmghannam can you please take a final look and see whether all is good?

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.

3 participants