Skip to content

Conversation

Joao-Dionisio
Copy link
Member

Fix #969

There's still a bug with getCol, but that one is also a bug in the normal variables. I'm thinking it may just be about adding an assertion.

@DominikKamp do you know when SCIPvarGetCol returns NULL?

@DominikKamp
Copy link
Contributor

Ideally, if there is no column to the variable for example if the variable is deleted already or no LP created yet.

@Joao-Dionisio
Copy link
Member Author

Joao-Dionisio commented Mar 25, 2025

@DominikKamp would it make sense to add a precondition to the method, even though it takes in SCIP_VAR* and not SCIP*? Or is returning NULL something to be expected? In the latter case, something should be said in the documentation, IMO.

@DominikKamp
Copy link
Contributor

Maybe you meant to check the upper bounds of the columns after optimizing but you might need to disable presolving before.

@DominikKamp
Copy link
Contributor

DominikKamp commented Mar 25, 2025

Returning NULL would be okay but there is also an assertion that the variable must have a column which should fail before.

@DominikKamp
Copy link
Contributor

The precondition can be that the variable is in the LP.

@mmghannam mmghannam requested a review from Copilot March 25, 2025 14:12
Copy link
Contributor

@Copilot 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 addresses a bug related to accessing matrix variable attributes. The changes include:

  • Adding a new test function (test_MatrixVariable_attributes) to verify multiple matrix variable attributes.
  • Adjusting the changelog to reflect the bug fix.

Reviewed Changes

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

File Description
tests/test_matrix_variable.py Introduces tests to verify methods for matrix variable attributes.
CHANGELOG.md Updates the changelog with the bug fix description.
Files not reviewed (1)
  • src/pyscipopt/scip.pxi: Language not supported
Comments suppressed due to low confidence (1)

tests/test_matrix_variable.py:303

  • The test for getCol is commented out, leaving its behavior untested; consider adding a TODO or reference to the bug tracking issue (#969) to clarify why this test is disabled and outline plans for future validation.
#assert x.getCol().tolist()# == [[5, 6], [2, 8]]

@Joao-Dionisio
Copy link
Member Author

Ah no, I didn't mean to check the upper bounds of the columns, the rhs of the check was there from copy-pasting. I just wanted to access the method. I think the error is explicit enough.

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.

Looks good, thanks @Joao-Dionisio!

@mmghannam mmghannam enabled auto-merge March 25, 2025 15:35
@mmghannam mmghannam merged commit e52b08d into master Mar 25, 2025
1 check passed
@Joao-Dionisio Joao-Dionisio deleted the fix-matrixvar-attributes branch March 26, 2025 13:50
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.

Matrix API easy access to LocalBounds

3 participants