Skip to content

Conversation

@mmghannam
Copy link
Member

@mmghannam mmghannam commented May 14, 2025

Without this, the value could be outdated as it depends on calling getBestSol before.

@mmghannam mmghannam requested a review from Joao-Dionisio May 14, 2025 10:42
@Joao-Dionisio
Copy link
Member

Joao-Dionisio commented May 14, 2025

Can you add a test that fails without this, please?

I'm a bit worried about scripts that call getVal very often, though. Sure, it's a tiny tiny additional effort, but people using things like getVarDict on big models might feel the difference. We would be creating a Solution object for every variable.

Can we somehow circumvent this? A terrible idea I just had was to attach an even handler to every model, that catches BESTSOLFOUND and the execmethod calls getBestSol.

@Joao-Dionisio
Copy link
Member

Joao-Dionisio commented May 14, 2025

image

This is cracking me up. If not raise error, then raise value error.
We were the ones who added this, Mo ahah

@mmghannam
Copy link
Member Author

Can you add a test that fails without this, please?

I'm a bit worried about scripts that call getVal very often, though. Sure, it's a tiny tiny additional effort, but people using things like getVarDict on big models might feel the difference. We would be creating a Solution object for every variable.

Can we somehow circumvent this? A terrible idea I just had was to attach an even handler to every model, that catches BESTSOLFOUND and the execmethod calls getBestSol.

We could maybe prevent it in the solving stage, and raise and error guiding the user to the other methods.

@Joao-Dionisio
Copy link
Member

what was it that we decided was the better way to deal with this @mmghannam ?

@marouane-f
Copy link

if not stage_check or self._bestSol.sol == NULL and SCIPgetStage(self._scip) != SCIP_STAGE_SOLVING:

The check allows to call getVal during solving, if a solution is available. Is this intended behavior?

@Joao-Dionisio
Copy link
Member

@mmghannam I remember we had a discussion about this, with a better way to do this. Do you remember what it was?

I'm not too fond of preventing it in the solving stage, since the intended behavior seems clear, and crashing the user's code seems somewhat frustrating.

@mmghannam
Copy link
Member Author

I unfortunately don't remember. I would also lean on the side of allowing it in the solving stage. Regarding the eventhandler idea, this would add an overhead to all models, which is not so nice. But either this solution or the eventhandler, any overhead is worth not returning an outdated value.

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