Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Feature/xpress solver for mathopt #137
base: main
Are you sure you want to change the base?
WIP: Feature/xpress solver for mathopt #137
Changes from 1 commit
c648ee7
073e8e3
6a6d3c0
95357b3
19805c0
d15a1b6
d825a91
d0502ea
dc317c0
62e6b26
38a13ba
1fb56d8
c9419e7
0ae4346
63a2a87
823a95d
b3d57f8
127e691
f3ffa94
6796e77
46e755a
1637a76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory
XPRSgetlasterror()
can fail. To make this rock solid I suggest to check the return value of that function and seterrmsg
to some generic string in case the function returns non-zero.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
const_cast
? There is no need to cast away theconst
sinceXPRSchgobj()
takesconst
arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no need to cast away
const
. You should be able to directly passrowind.data()
etc. toXPRSchgmcoef()
.Here I suggest to use
XPRSchgmcoef64()
. All data is already in the right format. You only have to changen_coefs
to a 64bit integer. This then supports 64bit non-zero counts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for the LP optimizing function above.
XPRSmipoptimize()
looks a bit fishy sinceXPRSoptimize()
should do what you want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which context is this function supposed to be called? There are only very special situations (inside callbacks) in which you would want to use
XPRSgetlpvalues()
to get the x vector. You most likely want to use XPRSgetsolution() here. This also has the advantage that it returns a status that indicates whether a solution is available at all.