Conversation
fwesselm
left a comment
fwesselm
left a comment
There was a problem hiding this comment.
Some more comments from me.
| return std::make_pair(feasible, transformed_solobj); | ||
| } | ||
|
|
||
| bool HighsMipWorker::trySolution(const std::vector<double>& solution, |
There was a problem hiding this comment.
Is this a copy of HighsMipSolverData::trySolution?
There was a problem hiding this comment.
Yes...... It is just calling a different addIncumbent at the end of feasibility check (which requires the HighsMipWorker object). I could pass HighsMipWorker* worker = nullptr as a default argument, and call the global HighsMipSolverData::addIncumbent vs HighsMipWorker::addIncumbent depending on whether or not it's a nullptr. Any strong opinion for or against (or a different approach)?
There was a problem hiding this comment.
Maybe another thing to look at after the initial submission.
| return true; | ||
| } | ||
|
|
||
| std::pair<bool, double> HighsMipWorker::transformNewIntegerFeasibleSolution( |
There was a problem hiding this comment.
This looks like a modified version of HighsMipSolverData::transformNewIntegerFeasibleSolution. I guess it is not possible to re-use the existing code? Should this live in HighsMipSolverData?
There was a problem hiding this comment.
After looking into this again, I guess it is now possible to reuse some of the existing code (I think I may have gotten lazy at this point). It would need:
- Some
nullptrdefault argument like in the above point - Some new statistics added to
HighsMipWorker(repair LP iterations) - Some timer magic (delaying fixing all of this for now)
Would you want this to be unified into one function?
There was a problem hiding this comment.
We can make a note and look at it later.
| HighsDomain& localdom = heur.getLocalDomain(); | ||
| heur.setHeuristic(true); | ||
|
|
||
| std::vector<HighsInt> intcols_; |
There was a problem hiding this comment.
I don't understand this code segment (intcols_, intcols, this->intcols). What is it doing?
There was a problem hiding this comment.
I have now done this a few times: I don't want to add boolean switches at all points in the code, i.e., wherever intcols is accessed, so I simply define it once as a reference, which involves creating the thing it potentially is referencing, e.g., intcols_.
There might be some obvious C++ mechanic I could be using here.
| if (heur.getCurrentDepth() > targetdepth) { | ||
| if (!heur.backtrackUntilDepth(targetdepth)) { | ||
| lp_iterations += heur.getLocalLpIterations(); | ||
| worker.heur_stats.lp_iterations += heur.getLocalLpIterations(); |
There was a problem hiding this comment.
Not for now, but maybe worth having a getHeurLpIterations method at some point.
There was a problem hiding this comment.
That would return a reference to heur_stats.lp_iterations?
There was a problem hiding this comment.
Yes, I just saw similar updates in different places and wanted to ask.
| void undo(const HighsOptions& options, HighsSolution& solution, | ||
| HighsBasis& basis, const HighsInt report_col = -1) { | ||
| reductionValues.resetPosition(); | ||
| HighsBasis& basis, const HighsInt report_col = -1, |
There was a problem hiding this comment.
Why is this needed? Don't the workers have separate postsolve stacks?
There was a problem hiding this comment.
The workers don't have separate postsolve stacks. The current undo function is changing some global data structures, so you can't call undo on two different threads simultaneously.
This change is adding an argument so it does everything on copied data.
highs/presolve/HPresolve.cpp
Outdated
| mipsolver->mipdata_->implications.rebuild(model->num_col_, newColIndex, | ||
| newRowIndex); | ||
| mipsolver->mipdata_->cutpool = | ||
| // TODO: Find a sensible way to do this |
There was a problem hiding this comment.
Are you deleting the cut pool?
There was a problem hiding this comment.
Yup! This was done before and I didn't see a need to change the workflow.
I had to get creative here because the std::atomic in HighsCutPool. I didn't want to change cutpool to a ptr in HighsMipData either (wanted to keep it as a reference). I'm happy with any ideas here as I think the current thing looks ugly (even if it works).
|
@Opt-Mucca I've merged latest into this branch to see whether all the CI tests pass |
5 participants
No description provided.