Skip to content

Change addConsNode to (also) accept ExprCons#418

Merged
CGraczyk merged 4 commits intoscipopt:addConsLocalfrom
CharJon:extend-addConsNode
Mar 18, 2021
Merged

Change addConsNode to (also) accept ExprCons#418
CGraczyk merged 4 commits intoscipopt:addConsLocalfrom
CharJon:extend-addConsNode

Conversation

@CharJon
Copy link
Contributor

@CharJon CharJon commented Aug 24, 2020

Based on discussion in #416
To keep the prototype simple a new addExprConsNode method is added.
Do you prefer having addConsNode accept ExprCons only or both (Constraint and ExprCons)?

@fserra
Copy link
Collaborator

fserra commented Aug 25, 2020

thanks. I think it would make sense to factor out the code that creates the linear constraint since it also appears in _addLinCons

Regarding the name and if it should also be part of addConsNode I am unsure. I am pinging @mattmilten here.
So we have addCons and addPyCons for when the argument is an ExprCons or a Constraint, resp.
So for consistency, we could have addConsNode and addPyConsNode.
But then we also have addConsLocal and we would need to do the same there.

An alternative would be to remove addPyCons and let addCons handle both cases.

@CharJon
Copy link
Contributor Author

CharJon commented Aug 25, 2020

Personally I think the addPyCons naming is a bit confusing, as the Constraint objects are the ones actually having a pointer to a SCIPConstraint, while the ExprCons addCons expects as input don't. If you prefer having a single method for both cases I will restructure the code.

@mattmilten
Copy link
Collaborator

mattmilten commented Aug 25, 2020

I prefer having a nice and easy-to-use method addCons that just accepts whatever you throw at it - be it ExprCons or Constraint.
And the same also holds for addConsLocal, etc., of course.

@CharJon
Copy link
Contributor Author

CharJon commented Aug 25, 2020

I created a new method _createCons and made changes to addCons and addConsNode to avoid duplicate code. The current version of addConsNode works for Constraint and ExprCons. There a two TODOs in the code as I'm still unsure about how to handle the reference counting of SCIP. If you have any sources I could study to understand why and when PY_SCIP_CALL(SCIPreleaseCons(self._scip, &scip_cons)) and Py_INCREF(cons) are necessary please tell me.

@CharJon CharJon changed the title WIP: Change addConsNode to (also) accept ExprCons Change addConsNode to (also) accept ExprCons Nov 19, 2020
Copy link
Collaborator

@mattmilten mattmilten 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 to me. Just the CHANGELOG entry is missing.

@CGraczyk CGraczyk changed the base branch from master to addConsNode March 18, 2021 07:43
@CGraczyk CGraczyk changed the base branch from addConsNode to addConsLocal March 18, 2021 07:45
@CGraczyk CGraczyk merged commit 28c0a32 into scipopt:addConsLocal Mar 18, 2021
@SanielDous
Copy link

Did this find its way into master branch already? Would be really nice

@zzy-opt
Copy link

zzy-opt commented Aug 23, 2022

I am implementing the branch and price where I need to add a linear constraint to the newly created node in branching rule. I am facing the exact same problem stated here.
The suggested solution definitely can solve my problem. Howerever I don't see this updated version of addConsNode function exists. What goes wrong here? Isn't it successfully merged?

@SanielDous
Copy link

@zzy-opt In #416 there is some more recent information about this topic.

@zzy-opt
Copy link

zzy-opt commented Aug 23, 2022

@SanielDous Thank you. It seems this cannot be fixed in short term. Maybe I had better figure out how to use cython and change source code myself.

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.

6 participants