WIP: Feature/xpress solver for mathopt #137

pet-mit · 2024-11-28T13:02:54Z

No description provided.

ortools/math_opt/solvers/xpress_solver.cc

ortools/math_opt/solvers/xpress/g_xpress.cc

rma350 · 2024-12-02T15:14:46Z

ortools/math_opt/solvers/xpress/g_xpress.cc

+                             const absl::Span<const double> ub,
+                             const absl::Span<const char> vtype) {
+  CHECK_EQ(vind.size(), vval.size());
+  const int num_vars = static_cast<int>(lb.size());


we are not quite ready yet, but we are working up to int64 support (models with more than 2**31 variables). Not sure if xpress supports this, but if you do, maybe use int64_t instead of int here.

XPRESS does support this but through separate API calls of course (in this instance, XPRSaddcols64 instead of XPRSaddcols). If it's OK for you, I'll add a TODO here to look into it, and add support for int64 later (it will need some reflexion on how to support both 32 and 64 archs, maybe we'll wait for you to do it for gurobi and then copy)

Xpress supports only 64bit counts of non-zeros. Number of variables, rows, etc. are all limited to 32bit signed integers. Since you are not adding any non-zeros here, the code is already safe for the kind of 64bit stuff that Xpress supports.
I still suggest that instead of static_cast<int>(lb.size()) you use a function that checks for overflow (lb.size() larger than INT_MAX) and raises an error if a user tries to create that many variables. I guess such a function will come in handy in other places as well.

ortools/math_opt/solvers/xpress/g_xpress.cc

ortools/math_opt/solvers/xpress_solver_test.cc

ortools/math_opt/solvers/xpress_solver.cc

… some code.

… & iter limits)

…up the code.

djunglas

Thanks a million for doing this!
I made a first review pass over the implementation (not the tests). In my comments I assume that we want to code against a somewhat recent version of Xpress (that already has XPRSoptmize(), XPRSgetsolution(), XPRSgetduals(), ...).

djunglas · 2024-12-11T14:43:20Z

ortools/math_opt/solvers/xpress/g_xpress.cc

+    return absl::OkStatus();
+  }
+  char errmsg[512];
+  XPRSgetlasterror(xpress_model_, errmsg);


In theory XPRSgetlasterror() can fail. To make this rock solid I suggest to check the return value of that function and set errmsg to some generic string in case the function returns non-zero.

djunglas · 2024-12-11T14:45:55Z

ortools/math_opt/solvers/xpress/g_xpress.cc

+  bool correctlyLoaded = initXpressEnv();
+  CHECK(correctlyLoaded);
+  XPRSprob* model;
+  CHECK_EQ(kXpressOk, XPRScreateprob(model));


This looks wrong to me. XPRSprob already is a pointer type. I think this should read

XPRSprob model; CHECK_EQ(kXpressOk, XPRScreateprob(&model)); DCHECK(model != nullptr); return absl::WrapUnique(new Xpress(model));

We should try to only DCHECK if there is a performance reason to do so. In the common case, it is better to either just CHECK, or propagate a status error here. At least that is the convention we attempt to follow throughout the mathopt codebase (we are not 100% consistent).

Do we expect XPRSprob* to be nullptr if XPRScreateprob returns OK? It seems like things are already pretty busted in this case and we can just CHECK.

you are right, I guess this is redundant and with no actual debug value. I'll remove it.

djunglas · 2024-12-11T14:51:00Z

ortools/math_opt/solvers/xpress/g_xpress.cc

+                             const absl::Span<const double> ub,
+                             const absl::Span<const char> vtype) {
+  CHECK_EQ(vind.size(), vval.size());
+  const int num_vars = static_cast<int>(lb.size());


Xpress supports only 64bit counts of non-zeros. Number of variables, rows, etc. are all limited to 32bit signed integers. Since you are not adding any non-zeros here, the code is already safe for the kind of 64bit stuff that Xpress supports.
I still suggest that instead of static_cast<int>(lb.size()) you use a function that checks for overflow (lb.size() larger than INT_MAX) and raises an error if a user tries to create that many variables. I guess such a function will come in handy in other places as well.

djunglas · 2024-12-11T14:53:14Z

ortools/math_opt/solvers/xpress/g_xpress.cc

+
+  const int n_cols = static_cast<int>(colind.size());
+  auto c_colind = const_cast<int*>(colind.data());
+  auto c_values = const_cast<double*>(values.data());


Why const_cast? There is no need to cast away the const since XPRSchgobj() takes const arguments.

djunglas · 2024-12-11T14:55:18Z

ortools/math_opt/solvers/xpress/g_xpress.cc

+  const int n_coefs = static_cast<int>(rowind.size());
+  auto c_rowind = const_cast<int*>(rowind.data());
+  auto c_colind = const_cast<int*>(colind.data());
+  auto c_values = const_cast<double*>(values.data());


Again, no need to cast away const. You should be able to directly pass rowind.data() etc. to XPRSchgmcoef().
Here I suggest to use XPRSchgmcoef64(). All data is already in the right format. You only have to change n_coefs to a 64bit integer. This then supports 64bit non-zero counts.

djunglas · 2024-12-11T15:14:57Z

ortools/math_opt/solvers/xpress_solver.cc

+  return xpress_->AddConstrs(constraint_sense, constraint_rhs, constraint_rng);
+}
+
+absl::Status XpressSolver::AddSingleObjective(const ObjectiveProto& objective) {


What is this function supposed to do? At the moment (unless I am mistaken) it only changes the coefficients of variables that appear in objective. If a variable is not in objective then its objective coefficient won't be changed. In particular, it would not be zeroed. Or is objective always dense?

This function is only called once, in LoadModel, after AddNewVariables. AddNewVariables actually calls XPRSaddcols with a NULL objective (meaning 0 coefficients). Then, in fact, AddSingleObjective only changes updates non-zero coefficients.
(I have not added support for model update between solves yet)

djunglas · 2024-12-11T15:17:13Z

ortools/math_opt/solvers/xpress_solver.cc

+  }
+  // Post-solve
+  if (!(is_mip_ ? (xpress_status_ == XPRS_MIP_OPTIMAL)
+                : (xpress_status_ == XPRS_LP_OPTIMAL))) {


As I commented in the other file, the preferred way to solve is XPRSoptimize() and then just check the solve and solution status returned by this function.

djunglas · 2024-12-11T15:19:21Z

ortools/math_opt/solvers/xpress_solver.cc

+  int lp_iter_limit = parameters.has_iteration_limit()
+                          ? parameters.iteration_limit()
+                          : 2147483647;
+  // XPRESS default value for XPRS_BARITERLIMIT is 500


In order to get the default values, you should rather query the attributes right after calling XPRScreateprob(). This seems more future proof.

djunglas · 2024-12-11T15:20:26Z

ortools/math_opt/solvers/xpress_solver.cc

+}
+
+absl::StatusOr<double> XpressSolver::GetBestDualBound() const {
+  // TODO: setting LP primal value as best dual bound. Can this be improved?


There is XPRS_BESTBOUND for this.

is it the same? the documentation says this is set by the MIP optimization (I'm looking for LP bound)

I don't know what xpress does, but in general, for LP:

primal simplex does not give any dual bound until you have solved the problem to optimality

For dual simplex, if you complete phase one and enter phase two, you have a dual feasible solution which gives a dual bound.

With barrier, you don't really get anything until optimality, as you don't have primal/dual feasible solutions until the end.

Some solver specific work is typically needed to extract the above information.

@rma350 Thank you for your input, in fact my version was too simple. I corrected it in my last commit ("compute dual bound")

djunglas · 2024-12-11T15:22:09Z

ortools/math_opt/solvers/xpress_solver.cc

+
+bool XpressSolver::isFeasible() const {
+  if (is_mip_) {
+    return xpress_status_ == XPRS_MIP_OPTIMAL;


There is another status that indicates "feasible but not proven optimal": XPRS_MIP_SOLUTION, see here.

I'm focused on making LP problems work in this first PR, but I'll make this correction since I already started developing MIP support.

pet-mit · 2024-12-11T15:33:44Z

Thanks a million for doing this! I made a first review pass over the implementation (not the tests). In my comments I assume that we want to code against a somewhat recent version of Xpress (that already has XPRSoptmize(), XPRSgetsolution(), XPRSgetduals(), ...).

Hi Daniel!

Thanks a lot for you valuable review!
You are right, we also prefer to target newer versions of XPRESS, and I'm definitely not familiar with the newer vs the older API calls.
I will take all your comments into account; however I'm on my way to my annual holidays until january, so I will do this in 2025 :)

Best regards !
Peter

Add XPRESS solver support to MathOpt

c648ee7

rma350 reviewed Dec 2, 2024

View reviewed changes

ortools/math_opt/solvers/xpress_solver.cc Outdated Show resolved Hide resolved

rma350 reviewed Dec 2, 2024

View reviewed changes

pet-mit added 10 commits December 3, 2024 16:19

changes after review

073e8e3

simplification

6a6d3c0

add support for ranged constraints

95357b3

fix some tests

19805c0

Fix basis handling, and infeasible & unbounded LPs handling. Refactor…

d15a1b6

… some code.

Fix constraint basis. Clean-up test class.

d825a91

Activate some test suites and improve XpressSolver error handling

d0502ea

Add IIS test

dc317c0

Enable LpParameterTestParams and add LP parameters support (algorithm…

62e6b26

… & iter limits)

Enable all tests required by Google. Improve some features and clean …

38a13ba

…up the code.

djunglas reviewed Dec 11, 2024

View reviewed changes

pet-mit added 3 commits January 3, 2025 16:50

review from Daniel

1fb56d8

remove redundant DCHECK

c9419e7

compute dual bound

0ae4346

pet-mit requested review from flomnes and djunglas January 14, 2025 08:10

pet-mit added 8 commits January 14, 2025 18:20

add comments

63a2a87

Merge branch 'main' into feature/xpress_solver_for_mathopt

823a95d

add support for quadratic objectives

b3d57f8

fix compilation

127e691

activate more QP tests

f3ffa94

Merge branch 'main' into feature/xpress_solver_for_mathopt

6796e77

improvements to duals support

46e755a

improvements to duals support

1637a76

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Feature/xpress solver for mathopt #137

WIP: Feature/xpress solver for mathopt #137

pet-mit commented Nov 28, 2024

rma350 Dec 2, 2024

pet-mit Dec 3, 2024

djunglas Dec 11, 2024

djunglas left a comment

djunglas Dec 11, 2024

djunglas Dec 11, 2024

rma350 Jan 3, 2025

pet-mit Jan 3, 2025

djunglas Dec 11, 2024

djunglas Dec 11, 2024

djunglas Dec 11, 2024

djunglas Dec 11, 2024

pet-mit Jan 3, 2025

djunglas Dec 11, 2024

djunglas Dec 11, 2024

djunglas Dec 11, 2024

pet-mit Jan 3, 2025

rma350 Jan 3, 2025

pet-mit Jan 6, 2025 •

edited

Loading

djunglas Dec 11, 2024

pet-mit Jan 3, 2025

pet-mit commented Dec 11, 2024

WIP: Feature/xpress solver for mathopt #137

Are you sure you want to change the base?

WIP: Feature/xpress solver for mathopt #137

Conversation

pet-mit commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djunglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pet-mit Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pet-mit commented Dec 11, 2024

pet-mit Jan 6, 2025 •

edited

Loading