From f94a2c7dfe21bf01878051ff7f854b975101587e Mon Sep 17 00:00:00 2001 From: Julian Hall Date: Tue, 19 Nov 2024 16:43:41 +0000 Subject: [PATCH] Added tests for starts and indices to HighsSparseMatrix, and using them in writeLocalModel --- check/TestFilereader.cpp | 36 ++++++++++++++------ src/lp_data/Highs.cpp | 18 ++++++++-- src/model/HighsHessianUtils.cpp | 8 +++-- src/util/HighsMatrixUtils.cpp | 2 +- src/util/HighsSparseMatrix.cpp | 59 +++++++++++++++++++++++++++++++++ src/util/HighsSparseMatrix.h | 3 ++ 6 files changed, 110 insertions(+), 16 deletions(-) diff --git a/check/TestFilereader.cpp b/check/TestFilereader.cpp index 551718ab79..63fe50e8a2 100644 --- a/check/TestFilereader.cpp +++ b/check/TestFilereader.cpp @@ -361,10 +361,11 @@ TEST_CASE("filereader-dD2e", "[highs_filereader]") { TEST_CASE("writeLocalModel", "[highs_filereader]") { Highs h; - // h.setOptionValue("output_flag", dev_run); + h.setOptionValue("output_flag", dev_run); std::string write_model_file = "foo.mps"; HighsModel model; - HighsLp& lp = model.lp_;; + HighsLp& lp = model.lp_; + ; lp.num_col_ = 2; lp.num_row_ = 3; lp.col_cost_ = {8, 10}; @@ -374,29 +375,44 @@ TEST_CASE("writeLocalModel", "[highs_filereader]") { lp.a_matrix_.index_ = {0, 1, 2, 0, 1, 2}; lp.a_matrix_.value_ = {2, 3, 2, 2, 4, 1}; - // if (dev_run) - printf("\nModel with no column lower or upper bounds\n"); + if (dev_run) printf("\nModel with no column lower or upper bounds\n"); REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError); lp.col_lower_ = {0, 0}; - // if (dev_run) - printf("\nModel with no column upper bounds\n"); + if (dev_run) printf("\nModel with no column upper bounds\n"); REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError); lp.col_upper_ = {inf, inf}; // Model has no dimensions for a_matrix_, but these are set in - // writeLocalModel. - printf("\nModel with no column or row names\n"); + // writeLocalModel. + if (dev_run) printf("\nModel with no column or row names\n"); REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kWarning); lp.col_names_ = {"C0", "C1"}; lp.row_names_ = {"R0", "R1", "R2"}; - printf("\nModel with column and row names\n"); + if (dev_run) printf("\nModel with column and row names\n"); REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kOk); + // Introduce illegal start + if (dev_run) printf("\nModel with start entry > num_nz\n"); + lp.a_matrix_.start_ = {0, 7, 6}; + REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError); + + // Introduce illegal start + if (dev_run) printf("\nModel with start entry -1\n"); + lp.a_matrix_.start_ = {0, -1, 6}; + REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError); + lp.a_matrix_.start_ = {0, 3, 6}; + // Introduce illegal index + if (dev_run) printf("\nModel with index entry -1\n"); lp.a_matrix_.index_ = {0, -1, 2, 0, 1, 2}; REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError); - + + // Introduce illegal index + if (dev_run) printf("\nModel with index entry 3 >= num_row\n"); + lp.a_matrix_.index_ = {0, 1, 3, 0, 1, 2}; + REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError); + std::remove(write_model_file.c_str()); } diff --git a/src/lp_data/Highs.cpp b/src/lp_data/Highs.cpp index dd5049b876..67c6d44e33 100644 --- a/src/lp_data/Highs.cpp +++ b/src/lp_data/Highs.cpp @@ -705,18 +705,32 @@ HighsStatus Highs::writePresolvedModel(const std::string& filename) { HighsStatus Highs::writeLocalModel(HighsModel& model, const std::string& filename) { HighsStatus return_status = HighsStatus::kOk; + HighsStatus call_status; HighsLp& lp = model.lp_; // Dimensions in a_matrix_ may not be set, so take them from lp lp.setMatrixDimensions(); + // Ensure that the LP is column-wise lp.ensureColwise(); + // Ensure that the dimensions are OK - if (!lpDimensionsOk("writeLocalModel", lp, options_.log_options)) return HighsStatus::kError; + if (!lpDimensionsOk("writeLocalModel", lp, options_.log_options)) + return HighsStatus::kError; + if (model.hessian_.dim_ > 0) { - HighsStatus call_status = assessHessianDimensions(options_, model.hessian_); + call_status = assessHessianDimensions(options_, model.hessian_); if (call_status == HighsStatus::kError) return call_status; } + + // Check that the matrix starts are OK + call_status = lp.a_matrix_.assessStart(options_.log_options); + if (call_status == HighsStatus::kError) return call_status; + + // Check that the matrix indices are within bounds + call_status = lp.a_matrix_.assessIndexBounds(options_.log_options); + if (call_status == HighsStatus::kError) return call_status; + // Check for repeated column or row names that would corrupt the file if (model.lp_.col_hash_.hasDuplicate(model.lp_.col_names_)) { highsLogUser(options_.log_options, HighsLogType::kError, diff --git a/src/model/HighsHessianUtils.cpp b/src/model/HighsHessianUtils.cpp index aad2a8c900..56a68f1543 100644 --- a/src/model/HighsHessianUtils.cpp +++ b/src/model/HighsHessianUtils.cpp @@ -28,7 +28,8 @@ HighsStatus assessHessian(HighsHessian& hessian, const HighsOptions& options) { HighsStatus return_status = HighsStatus::kOk; HighsStatus call_status; - return_status = interpretCallStatus(options.log_options, assessHessianDimensions(options, hessian), + return_status = interpretCallStatus(options.log_options, + assessHessianDimensions(options, hessian), return_status, "assessHessianDimensions"); if (return_status == HighsStatus::kError) return return_status; @@ -111,8 +112,9 @@ HighsStatus assessHessianDimensions(const HighsOptions& options, // Assess the Hessian dimensions and vector sizes vector hessian_p_end; const bool partitioned = false; - return assessMatrixDimensions(options.log_options, hessian.dim_, partitioned, hessian.start_, - hessian_p_end, hessian.index_, hessian.value_); + return assessMatrixDimensions(options.log_options, hessian.dim_, partitioned, + hessian.start_, hessian_p_end, hessian.index_, + hessian.value_); } void completeHessianDiagonal(const HighsOptions& options, diff --git a/src/util/HighsMatrixUtils.cpp b/src/util/HighsMatrixUtils.cpp index b55f4df79a..7b70695065 100644 --- a/src/util/HighsMatrixUtils.cpp +++ b/src/util/HighsMatrixUtils.cpp @@ -68,7 +68,7 @@ HighsStatus assessMatrix( // // Check whether the first start is zero if (matrix_start[0]) { - highsLogUser(log_options, HighsLogType::kWarning, + highsLogUser(log_options, HighsLogType::kError, "%s matrix start vector begins with %" HIGHSINT_FORMAT " rather than 0\n", matrix_name.c_str(), matrix_start[0]); diff --git a/src/util/HighsSparseMatrix.cpp b/src/util/HighsSparseMatrix.cpp index a493d2e7ad..d063f0468e 100644 --- a/src/util/HighsSparseMatrix.cpp +++ b/src/util/HighsSparseMatrix.cpp @@ -722,6 +722,65 @@ void HighsSparseMatrix::deleteRows( this->num_row_ = new_num_row; } +HighsStatus HighsSparseMatrix::assessStart(const HighsLogOptions& log_options) { + // Identify main dimensions + HighsInt vec_dim; + HighsInt num_vec; + if (this->isColwise()) { + vec_dim = this->num_row_; + num_vec = this->num_col_; + } else { + vec_dim = this->num_col_; + num_vec = this->num_row_; + } + if (this->start_[0]) { + highsLogUser(log_options, HighsLogType::kError, + "Matrix start[0] = %d, not 0\n", int(this->start_[0])); + return HighsStatus::kError; + } + HighsInt num_nz = this->numNz(); + for (HighsInt iVec = 1; iVec < num_vec; iVec++) { + if (this->start_[iVec] < this->start_[iVec - 1]) { + highsLogUser(log_options, HighsLogType::kError, + "Matrix start[%d] = %d > %d = start[%d]\n", int(iVec), + int(this->start_[iVec]), int(this->start_[iVec - 1]), + int(iVec - 1)); + return HighsStatus::kError; + } + if (this->start_[iVec] > num_nz) { + highsLogUser(log_options, HighsLogType::kError, + "Matrix start[%d] = %d > %d = number of nonzeros\n", + int(iVec), int(this->start_[iVec]), int(num_nz)); + return HighsStatus::kError; + } + } + return HighsStatus::kOk; +} + +HighsStatus HighsSparseMatrix::assessIndexBounds( + const HighsLogOptions& log_options) { + // Identify main dimensions + HighsInt vec_dim; + HighsInt num_vec; + if (this->isColwise()) { + vec_dim = this->num_row_; + // num_vec = this->num_col_; + } else { + vec_dim = this->num_col_; + // num_vec = this->num_row_; + } + HighsInt num_nz = this->numNz(); + for (HighsInt iEl = 1; iEl < num_nz; iEl++) { + if (this->index_[iEl] < 0 || this->index_[iEl] >= vec_dim) { + highsLogUser(log_options, HighsLogType::kError, + "Matrix index[%d] = %d is not in legal range of [0, %d)\n", + int(iEl), int(this->index_[iEl]), vec_dim); + return HighsStatus::kError; + } + } + return HighsStatus::kOk; +} + HighsStatus HighsSparseMatrix::assess(const HighsLogOptions& log_options, const std::string matrix_name, const double small_matrix_value, diff --git a/src/util/HighsSparseMatrix.h b/src/util/HighsSparseMatrix.h index 7333a37e64..9694183191 100644 --- a/src/util/HighsSparseMatrix.h +++ b/src/util/HighsSparseMatrix.h @@ -66,6 +66,9 @@ class HighsSparseMatrix { void deleteRows(const HighsIndexCollection& index_collection); HighsStatus assessDimensions(const HighsLogOptions& log_options, const std::string matrix_name); + HighsStatus assessStart(const HighsLogOptions& log_options); + HighsStatus assessIndexBounds(const HighsLogOptions& log_options); + HighsStatus assess(const HighsLogOptions& log_options, const std::string matrix_name, const double small_matrix_value,