Update all objective coefficients in one time

[flomnes]

Short description

This PR is about changing the behavior of XpressInterface::SetObjectiveCoefficient, the function which is used to set the objective coefficient for a single variable. This approach proves to be much faster than individual updates.

Before

XpressInterface::SetObjectiveCoefficient immediately changes the coefficients by calling

int const col = variable->index();
CHECK_STATUS(XPRSchgobj(mLp, 1, &col, &coefficient));

After

The model is invalidated by calling InvalidateModelSynchronization. When calling MPSolver::Solve, all coefficients are extracted in one time

void XpressInterface::ExtractObjective() {
  // NOTE: The code assumes that the objective expression does not contain
  //       any non-zero duplicates.

  int const cols = XPRSgetnumcols(mLp);
  // DCHECK_EQ(last_variable_index_, cols);

  unique_ptr<int[]> ind(new int[cols]);
  unique_ptr<double[]> val(new double[cols]);
  for (int j = 0; j < cols; ++j) {
    ind[j] = j;
    val[j] = 0.0;
  }

  const auto& coeffs = solver_->objective_->coefficients_;
  for (auto it = coeffs.begin(); it != coeffs.end(); ++it) {
    int const idx = it->first->index();
    if (variable_is_extracted(idx)) {
      DCHECK_LT(idx, cols);
      val[idx] = it->second;
    }
  }

  CHECK_STATUS(XPRSchgobj(mLp, cols, ind.get(), val.get()));
  CHECK_STATUS(XPRSsetobjoffset(mLp, solver_->Objective().offset()));
}

[pet-mit]

@flomnes Is this ready for review? If yes, can we have a brief call? because I'm not sure I understand how the SlowUpdates mechanism works

[djunglas]

This can be improved, IMO.
If you replace

val[idx] = it->second;

by

val[idx] += it->second;

(note += instead of =) then the code is correct even if the objective expression contains duplicates.

XPRSsetobjoffset() does not exist. In order to change the constant term in the objective use index -1, see the reference documentation.

The code is rather inefficient if the objective is not dense. How about changing the code to something like this to create sparse arrays for calling XPRSchgobj():

unique_ptr<int[]> ind(new int[cols + 1];
unique_ptr<double[]> val(new double[cols + 1]);
unique_ptr<int[]> map(new int[cols]);
memset(map.get(), -1, sizeof(*map.get()) * cols); // set all elements in map to -1
int count = 0;

const auto& coeffs = solver_->objective_->coefficients_;
for (auto it = coeffs.begin(); it != coeffs.end(); ++it) {
  int const idx = it->first->index();
  if (variable_is_extracted(idx)) {
    DCHECK_LT(idx, cols);
    if (map[idx] >= 0) { // We already saw this variable
      val[map[idx]] += it->second;
    }
    else { // First time we see this variable
      map[idx] = count;
      ind[count] = idx;
      val[count] = it->second;
      ++count;
    }
  }
}
ind[count] = -1;
val[count] = solver_->Objective().offset();
++count;

CHECK_STATUS(XPRSchgobj(mLp, count, ind.get(), val.get()));

[flomnes]

XPRSsetobjoffset() does not exist. In order to change the constant term in the objective use index -1, see the reference documentation.

This is actually our own function, though I admit the naming is a bit confusing and should be changed

or-tools/ortools/linear_solver/xpress_interface.cc

Lines 88 to 94 in 8564311

	int XPRSsetobjoffset(const XPRSprob& mLp, double value) {
	// TODO detect xpress version
	static int indexes[1] = { -1 };
	double values[1] = { -value };
	XPRSchgobj(mLp, 1, indexes, values);
	return 0;
	}

[pet-mit]

This can be improved, IMO. If you replace

val[idx] = it->second;

by

val[idx] += it->second;

(note += instead of =) then the code is correct even if the objective expression contains duplicates.

XPRSsetobjoffset() does not exist. In order to change the constant term in the objective use index -1, see the reference documentation.

The code is rather inefficient if the objective is not dense. How about changing the code to something like this to create sparse arrays for calling XPRSchgobj():

unique_ptr<int[]> ind(new int[cols + 1];
unique_ptr<double[]> val(new double[cols + 1]);
unique_ptr<int[]> map(new int[cols]);
memset(map.get(), -1, sizeof(*map.get()) * cols); // set all elements in map to -1
int count = 0;

const auto& coeffs = solver_->objective_->coefficients_;
for (auto it = coeffs.begin(); it != coeffs.end(); ++it) {
  int const idx = it->first->index();
  if (variable_is_extracted(idx)) {
    DCHECK_LT(idx, cols);
    if (map[idx] >= 0) { // We already saw this variable
      val[map[idx]] += it->second;
    }
    else { // First time we see this variable
      map[idx] = count;
      ind[count] = idx;
      val[count] = it->second;
      ++count;
    }
  }
}
ind[count] = -1;
val[count] = solver_->Objective().offset();
++count;

CHECK_STATUS(XPRSchgobj(mLp, count, ind.get(), val.get()));

Thank you Daniel, how about opening a new PR to take into account your proposal.
I think that @flomnes 's development here, for this specific feature, is not impacted by it.

[djunglas]

This can be improved, IMO. If you replace

val[idx] = it->second;

by

val[idx] += it->second;

(note += instead of =) then the code is correct even if the objective expression contains duplicates.
XPRSsetobjoffset() does not exist. In order to change the constant term in the objective use index -1, see the reference documentation.
The code is rather inefficient if the objective is not dense. How about changing the code to something like this to create sparse arrays for calling XPRSchgobj():

unique_ptr<int[]> ind(new int[cols + 1];
unique_ptr<double[]> val(new double[cols + 1]);
unique_ptr<int[]> map(new int[cols]);
memset(map.get(), -1, sizeof(*map.get()) * cols); // set all elements in map to -1
int count = 0;

const auto& coeffs = solver_->objective_->coefficients_;
for (auto it = coeffs.begin(); it != coeffs.end(); ++it) {
  int const idx = it->first->index();
  if (variable_is_extracted(idx)) {
    DCHECK_LT(idx, cols);
    if (map[idx] >= 0) { // We already saw this variable
      val[map[idx]] += it->second;
    }
    else { // First time we see this variable
      map[idx] = count;
      ind[count] = idx;
      val[count] = it->second;
      ++count;
    }
  }
}
ind[count] = -1;
val[count] = solver_->Objective().offset();
++count;

CHECK_STATUS(XPRSchgobj(mLp, count, ind.get(), val.get()));

Thank you Daniel, how about opening a new PR to take into account your proposal. I think that @flomnes 's development here, for this specific feature, is not impacted by it.

Fine with me. The code is not wrong by itself, so no need to hold back the merge for this.