From 02b17e3df9ffceeb4aaf953a60bbb414ada0dfe3 Mon Sep 17 00:00:00 2001 From: Laurent Perron Date: Fri, 20 Jul 2018 10:04:38 -0700 Subject: [PATCH] remove unnecessary refactorisation in glop --- ortools/glop/reduced_costs.cc | 5 ++- ortools/glop/revised_simplex.cc | 70 ++++++++++++--------------------- ortools/glop/revised_simplex.h | 16 +------- 3 files changed, 30 insertions(+), 61 deletions(-) diff --git a/ortools/glop/reduced_costs.cc b/ortools/glop/reduced_costs.cc index 21375e6bea..00ca0c0458 100644 --- a/ortools/glop/reduced_costs.cc +++ b/ortools/glop/reduced_costs.cc @@ -101,8 +101,9 @@ bool ReducedCosts::TestEnteringReducedCostPrecision( stats_.reduced_costs_accuracy.Add(estimated_reduced_costs_accuracy / scale); if (std::abs(estimated_reduced_costs_accuracy) / scale > parameters_.recompute_reduced_costs_threshold()) { - VLOG(1) << "Recomputing reduced costs: " << precise_reduced_cost << " vs " - << old_reduced_cost; + VLOG(1) << "Recomputing reduced costs, value = " << precise_reduced_cost + << " error = " + << std::abs(precise_reduced_cost - old_reduced_cost); MakeReducedCostsPrecise(); } } diff --git a/ortools/glop/revised_simplex.cc b/ortools/glop/revised_simplex.cc index 4480963d95..71e1a8e408 100644 --- a/ortools/glop/revised_simplex.cc +++ b/ortools/glop/revised_simplex.cc @@ -1373,17 +1373,6 @@ Fractional RevisedSimplex::ComputeDirectionError(ColIndex col) { return InfinityNorm(error_); } -void RevisedSimplex::SkipVariableForRatioTest(RowIndex row) { - // Setting direction_[row] to 0.0 is an effective way to ignore the row - // during the ratio test. The direction vector will be restored later from - // the information in direction_ignored_position_. - IF_STATS_ENABLED( - ratio_test_stats_.abs_skipped_pivot.Add(std::abs(direction_[row]))); - VLOG(1) << "Skipping leaving variable with coefficient " << direction_[row]; - direction_ignored_position_.SetCoefficient(row, direction_[row]); - direction_[row] = 0.0; -} - template Fractional RevisedSimplex::GetRatio(RowIndex row) const { const ColIndex col = basis_[row]; @@ -1421,10 +1410,18 @@ Fractional RevisedSimplex::ComputeHarrisRatioAndLeavingCandidates( // bound-flip over such leaving variable. Fractional harris_ratio = bound_flip_ratio; leaving_candidates->Clear(); - const Fractional threshold = parameters_.ratio_test_zero_threshold(); + + // If the basis is refactorized, then we should have everything with a good + // precision, so we only consider "acceptable" pivots. Otherwise we consider + // all the entries, and if the algorithm return a pivot that is too small, we + // will refactorize and recompute the relevant quantities. + const Fractional threshold = basis_factorization_.IsRefactorized() + ? parameters_.minimum_acceptable_pivot() + : parameters_.ratio_test_zero_threshold(); + for (const RowIndex row : direction_.non_zeros) { const Fractional magnitude = std::abs(direction_[row]); - if (magnitude < threshold) continue; + if (magnitude <= threshold) continue; Fractional ratio = GetRatio(row); // TODO(user): The perturbation is currently disabled, so no need to test // anything here. @@ -1486,7 +1483,6 @@ Status RevisedSimplex::ChooseLeavingVariableRow( DCHECK_NE(0.0, reduced_cost); // A few cases will cause the test to be recomputed from the beginning. - direction_ignored_position_.Clear(); int stats_num_leaving_choices = 0; equivalent_leaving_choices_.clear(); while (true) { @@ -1597,41 +1593,31 @@ Status RevisedSimplex::ChooseLeavingVariableRow( // TODO(user): We may have to choose another entering column if // we cannot prevent pivoting by a small pivot. // (Chvatal, p.115, about epsilon2.) - // - // Note(user): As of May 2013, just checking the pivot size is not - // preventing the algorithm to run into a singular basis in some rare cases. - // One way to be more precise is to also take into account the norm of the - // direction. if (pivot_magnitude < parameters_.small_pivot_threshold() * direction_infinity_norm_) { - VLOG(1) << "Trying not to pivot by " << direction_[*leaving_row] - << " direction_infinity_norm_ = " << direction_infinity_norm_; - // The first countermeasure is to recompute everything to the best // precision we can in the hope of avoiding such a choice. Note that this // helps a lot on the Netlib problems. if (!basis_factorization_.IsRefactorized()) { + VLOG(1) << "Refactorizing to avoid pivoting by " + << direction_[*leaving_row] + << " direction_infinity_norm_ = " << direction_infinity_norm_ + << " reduced cost = " << reduced_cost; *refactorize = true; return Status::OK(); } - // Note(user): This reduces quite a bit the number of iterations. - // What is its impact? Is it dangerous? - if (std::abs(direction_[*leaving_row]) < - parameters_.minimum_acceptable_pivot()) { - SkipVariableForRatioTest(*leaving_row); - continue; - } - - // TODO(user): in almost all cases, testing the pivot is not useful - // because the two countermeasures above will be enough. Investigate - // more. The false makes sure that this will just be compiled away. - if (/* DISABLES CODE */ (false) && - /* DISABLES CODE */ (!TestPivot(entering_col, *leaving_row))) { - SkipVariableForRatioTest(*leaving_row); - continue; - } - + // Because of the "threshold" in ComputeHarrisRatioAndLeavingCandidates() + // we kwnow that this pivot will still have an acceptable magnitude. + // + // TODO(user): An issue left to fix is that if there is no such pivot at + // all, then we will report unbounded even if this is not really the case. + // As of 2018/07/18, this happens on l30.mps. + VLOG(1) << "Couldn't avoid pivoting by " << direction_[*leaving_row] + << " direction_infinity_norm_ = " << direction_infinity_norm_ + << " reduced cost = " << reduced_cost; + DCHECK_GE(std::abs(direction_[*leaving_row]), + parameters_.minimum_acceptable_pivot()); IF_STATS_ENABLED(ratio_test_stats_.abs_tested_pivot.Add(pivot_magnitude)); } break; @@ -1646,12 +1632,6 @@ Status RevisedSimplex::ChooseLeavingVariableRow( : lower_bound_[basis_[*leaving_row]]; } - // Revert the temporary modification to direction_. - // This is important! Otherwise we would propagate some artificial errors. - for (const SparseColumn::Entry e : direction_ignored_position_) { - direction_[e.row()] = e.coefficient(); - } - // Stats. IF_STATS_ENABLED({ ratio_test_stats_.leaving_choices.Add(stats_num_leaving_choices); diff --git a/ortools/glop/revised_simplex.h b/ortools/glop/revised_simplex.h index 3f0e94bbfa..b18b388de6 100644 --- a/ortools/glop/revised_simplex.h +++ b/ortools/glop/revised_simplex.h @@ -413,15 +413,8 @@ class RevisedSimplex { void ComputeDirection(ColIndex col); // Computes a - B.d in error_ and return the maximum std::abs() of its coeffs. - // TODO(user): Use this to trigger a refactorization of B? Or to track the - // error created by calls to SkipVariableForRatioTest()? Fractional ComputeDirectionError(ColIndex col); - // Marks the corresponding basic variable so that it will not take part in the - // ratio test and will never be chosen as a leaving variable. This is used - // to avoid degenerate pivots. - void SkipVariableForRatioTest(RowIndex row); - // Computes the ratio of the basic variable corresponding to 'row'. A target // bound (upper or lower) is choosen depending on the sign of the entering // reduced cost and the sign of the direction 'd_[row]'. The ratio is such @@ -672,11 +665,6 @@ class RevisedSimplex { ScatteredColumn direction_; Fractional direction_infinity_norm_; - // Subpart of direction_ that was ignored during the ratio test. This is only - // used for degenerate problem. See SkipVariableForRatioTest() for more - // details. - SparseColumn direction_ignored_position_; - // Used to compute the error 'b - A.x' or 'a - B.d'. DenseColumn error_; @@ -814,8 +802,8 @@ class RevisedSimplexDictionary { RevisedSimplexDictionary(const DenseRow* col_scales, RevisedSimplex* revised_simplex) : dictionary_( - CHECK_NOTNULL(revised_simplex)->ComputeDictionary(col_scales)), - basis_vars_(CHECK_NOTNULL(revised_simplex)->GetBasisVector()) {} + ABSL_DIE_IF_NULL(revised_simplex)->ComputeDictionary(col_scales)), + basis_vars_(ABSL_DIE_IF_NULL(revised_simplex)->GetBasisVector()) {} ConstIterator begin() const { return dictionary_.begin(); } ConstIterator end() const { return dictionary_.end(); }