scipopt / scip

SCIP - Solving Constraint Integer Programs
Other
406 stars 67 forks source link

Coloring application Branching Rule contradictory code #77

Closed meffertj closed 2 months ago

meffertj commented 11 months ago

Hello,

The coloring branch and price application documentation says it selects the "most" fractional variable (both in the doc and in the branch_coloring.c itself) but in the actual code it selects the "least" fractional variable, even stating so in the comment:

https://github.com/scipopt/scip/blob/6d9a5d4b96030945e8f620bf03887a59e0bd31a9/applications/Coloring/src/branch_coloring.c#L141

meffertj commented 11 months ago

I could do a pull request editing the mentioned code segment to indeed select the most fractional node

ambros-gleixner commented 11 months ago

Yes, that would be wonderful. Thank you for noticing!

If you wanted to then you could even introduce a parameter to be able to switch between most and least fractional. But I believe that least fractional leads to better performance in practice.

meffertj commented 11 months ago

Alright, I changed the section in my fork. https://github.com/meffertj/scip_vcp_mostfractional/blob/master/applications/Coloring/src/branch_coloring.c

Do you have a recommendation as to what the most elegant place to introduce that parameter would be?

ambros-gleixner commented 11 months ago

Yes, the include method of the branching rule would be the natural place to do so, see https://github.com/meffertj/scip_vcp_mostfractional/blob/master/src/scip/branch_pscost.c#L777 for one example.

ambros-gleixner commented 11 months ago

A character parameter from the string "ml" might be a natural choice.

leoneifler commented 9 months ago

Is there any progress on this?

meffertj commented 9 months ago

I‘ve implemented it. My windows machine died, have since been working on my mac and was not able to test it yet

meffertj commented 3 months ago

I've fixed one error, now all tests run successfully - correctly solving when selecting the strategy via {m,l} and exiting with error message when not passing one of ml. I am not sure if it is justified to add myself as an author to the file, depending on your approval I can do a pull request

ambros-gleixner commented 2 months ago

Great, thank you very much! It is absolutely good to add you as an author. I have transferred your branch to the internal repo where we merge and run the CI. @GioniMexi will be in contact with you regarding this!

ambros-gleixner commented 2 months ago

Your code has been merged and should be available on the master branch now. Can you confirm and close the comment if all looks good? Thank you again for your contribution!

meffertj commented 2 months ago

Looks good, thank you!