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.

Sign up for GitHub

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

Jump to bottom

SCIPBackend: Faster copy, remove_constraint methods #35103

Merged
merged 10 commits into from Feb 25, 2024
Merged

SCIPBackend: Faster copy, remove_constraint methods #35103

merged 10 commits into from Feb 25, 2024

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Feb 13, 2023 •
edited

πŸ“š Description

Fixes #34890

Authors: @mkoeppe, @mantepse

πŸ“ Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

βŒ› Dependencies

@mkoeppe mkoeppe added this to the sage-10.0 milestone Feb 13, 2023
@mkoeppe mkoeppe mentioned this pull request Feb 13, 2023
Closed
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023 •
edited

Codecov Report

Base: 88.58% // Head: 88.58% // Increases project coverage by +0.00% πŸŽ‰

Coverage data is based on head (fd00880) compared to base (293dd72).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop   #35103   +/-   ##
========================================
  Coverage    88.58%   88.58%           
========================================
  Files         2140     2140           
  Lines       396961   396961           
========================================
+ Hits        351655   351665   +10     
+ Misses       45306    45296   -10
Impacted Files Coverage Ξ”
src/sage/quadratic_forms/random_quadraticform.py 90.00% <0.00%> (-6.00%) ⬇️
src/sage/interfaces/qsieve.py 71.30% <0.00%> (-2.61%) ⬇️
src/sage/cpython/_py2_random.py 74.79% <0.00%> (-1.24%) ⬇️
src/sage/graphs/generators/random.py 91.26% <0.00%> (-1.03%) ⬇️
src/sage/modular/modform/numerical.py 94.19% <0.00%> (-0.65%) ⬇️
src/sage/quadratic_forms/ternary_qf.py 66.73% <0.00%> (-0.59%) ⬇️
src/sage/sets/integer_range.py 91.41% <0.00%> (-0.51%) ⬇️
src/sage/graphs/generic_graph.py 89.12% <0.00%> (-0.40%) ⬇️
src/sage/schemes/elliptic_curves/cardinality.py 87.00% <0.00%> (-0.40%) ⬇️
src/sage/modular/arithgroup/arithgroup_perm.py 92.57% <0.00%> (-0.38%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

β˜” View full report at Codecov.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@mkoeppe mkoeppe requested a review from tscrim February 14, 2023 19:57
tscrim
tscrim requested changes Feb 15, 2023
View reviewed changes
src/sage/numerical/backends/scip_backend.pyx Outdated Show resolved Hide resolved
src/sage/numerical/backends/scip_backend.pyx
Comment on lines +852 to +848
if self.constraints is None:
return self.model.getNConss()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a doctest showing this code path?

src/sage/numerical/backends/scip_backend.pyx
1
sage: p.add_linear_constraint([(0, 2), (1, 3)], None, 6)
sage: p.add_linear_constraint([(0, 3), (1, 2)], None, 6)
sage: p.remove_constraints([0, 1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you extend this doctest to show the constraints have been removed?

It would also be good to have another doctest showing particular constraints (or just one) have been removed and the remaining are properly set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I'll add these tests

@mkoeppe mkoeppe self-assigned this Feb 18, 2023
@mkoeppe mkoeppe modified the milestones: sage-10.0, sage-10.1 Apr 30, 2023
@mkoeppe mkoeppe removed this from the sage-10.1 milestone Aug 7, 2023
@mkoeppe mkoeppe force-pushed the t/34890/scipbackend__faster_copy_method branch from fd00880 to 094beb3 Compare August 23, 2023 17:51
@tscrim
Copy link
Collaborator

tscrim commented Aug 24, 2023

Ready for another review?

@mantepse
Copy link
Collaborator

What is the status of this PR?

@mkoeppe mkoeppe requested a review from tscrim February 19, 2024 00:54
@github-actions GitHub Actions
Copy link

Documentation preview for this PR (built with commit cc55c9e; changes) is ready! πŸŽ‰

@mantepse
Copy link
Collaborator

Since, apparently, only very few people are using scip, I'll set this to positive review even though I have added some doctests myself.

mantepse
mantepse approved these changes Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@mantepse mantepse 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!

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 19, 2024

Positive review for your changes from my side. Thank you!

@vbraun vbraun merged commit be3a116 into sagemath:develop Feb 25, 2024
23 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
@mkoeppe mkoeppe deleted the t/34890/scipbackend__faster_copy_method branch May 3, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantepse mantepse mantepse approved these changes

@tscrim tscrim Awaiting requested review from tscrim

Assignees

@mkoeppe mkoeppe

Labels
c: linear programming c: packages: optional
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

SCIPBackend: Faster copy, remove_constraint methods
5 participants
@mkoeppe @codecov-commenter @tscrim @mantepse @vbraun