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.

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

Generic indicator names #930

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Generic indicator names #930

merged 4 commits into from
Dec 3, 2024

Conversation

Joao-Dionisio
Copy link
Collaborator

@Joao-Dionisio Joao-Dionisio commented Nov 30, 2024

Indicator constraints all had the same name by default. They now have the same default naming convention as other constraints.

I did discover a big bug that I'll fix later. When a model has indicator constraints and is optimized, all the constraints disappear. Not good. The tests are failing because of the assertion I added detecting this. EDIT: I'm very dumb, by default getNConss returns the number of constraints of the transformed problem.

@Opt-Mucca
Copy link
Collaborator

Two things:

  • This branch was built on your older branch that you changed (I assume). All the display problem stuff should be removed.
  • You shouldn't have a default name for the indicator cons. I like the two line change on changing the name if name=="" though and think it's needed

@Joao-Dionisio
Copy link
Collaborator Author

Joao-Dionisio commented Dec 3, 2024

@Opt-Mucca yep, I made an oopsie when branching.

As for your second point, the indicator constraints now have the same behavior as normal constraints - default name = "", and changing it to name = 'c'+str(SCIPgetNConss(self._scip)+1) if this is the case. You are saying that the name should be a required argument? also for the constraints?

@Joao-Dionisio Joao-Dionisio marked this pull request as ready for review December 3, 2024 11:42
@Opt-Mucca
Copy link
Collaborator

Opt-Mucca commented Dec 3, 2024

It is not the same as normal constraints. You have changed the default name to "indicatorCons", which will bypass your change to cX as a default name.

Edit: So my request change would be to have name="" in the function definition as in other functions.

Double edit: I don't know how the tests are passing...... Have I misunderstood something?

@Joao-Dionisio
Copy link
Collaborator Author

@Opt-Mucca I don't think so, I think I changed the default name from indicatorCons, which was there before, to "".

image

@Opt-Mucca
Copy link
Collaborator

I am blind and an idiot.............

@Opt-Mucca Opt-Mucca merged commit e7bf03c into master Dec 3, 2024
16 checks passed
@Joao-Dionisio Joao-Dionisio deleted the generic-indicator-names branch December 3, 2024 13:32
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.

2 participants