-
Notifications
You must be signed in to change notification settings - Fork 258
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
Fix wrong numbers for Event Types #759
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #759 +/- ##
==========================================
- Coverage 62.12% 61.52% -0.60%
==========================================
Files 16 16
Lines 3168 3236 +68
==========================================
+ Hits 1968 1991 +23
- Misses 1200 1245 +45 ☔ View full report in Codecov by Sentry. |
Oh nice! Yes, this is much better and way less error-prone. |
Thank you, Mathias! I think this should be working, @mmghannam, but I'm a bit wary of pushing this without doing proper tests on all these things. On the other hand, I suppose that it's difficult that this breaks even more stuff... There's also the matter of having a double declaration of |
Thanks a lot João! Let's add some tests and merge this straight away. |
…/PySCIPOpt into add-eventtype-Nodedelete
Okay @mmghannam, I think I added every event type now, a bunch were missing. As for the tests, I'm only testing whether the events that PySCIPOPt is catching and executing are the same. I could add a lot more sophisticated tests, but it will take a reaaaally long time to create a proper test for every event. PS: I really need Github Copilot... |
An important issue came to our attention. PySCIPOPt has been using very outdated identifiers for event types. This draft PR is an attempt to correct this.
I think we should do the same for all other types, not relying on the use of magic numbers.
The way I'm doing this is probably not the best, as defining a new SCIP_EVENTTYPE inside PySCIPOPt is quite convenient.
Also, almost none of these events are tested. This code passes the tests we have, but I can't guarantee that it's correct.