Skip to content
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

Update HiGHS to version 1.6.0 #35

Merged
merged 11 commits into from
Oct 10, 2023
Merged

Update HiGHS to version 1.6.0 #35

merged 11 commits into from
Oct 10, 2023

Conversation

fuglede
Copy link
Contributor

@fuglede fuglede commented Oct 9, 2023

This updates the HiGHS submodule to https://github.com/ERGO-Code/HiGHS/releases/tag/v1.6.0.

@lovasoa
Copy link
Owner

lovasoa commented Oct 9, 2023

This has been raised before. You need to update the solution parsing code if you want to upgrade highs

@lovasoa
Copy link
Owner

lovasoa commented Oct 9, 2023

see #29

@lovasoa
Copy link
Owner

lovasoa commented Oct 9, 2023

function parseResult(lines, status) {

This fixes an issue in the earlier commit; the property "isLinear" means
"MILP but not LP".
@jajhall
Copy link
Collaborator

jajhall commented Oct 9, 2023

Note that the original code use to parse the solution was the human-readable form. It's vastly better and easier to parse the computer-readable solution file, so I recommend that this is done if you're updating the solution parsing code

@lovasoa
Copy link
Owner

lovasoa commented Oct 9, 2023

@jajhall does the computer-readable format have the same information as the human-readable format in the latest versions ?

The information is rather superfluous, as, on one hand, the user will
likely be aware which model they are solving, and on the other, since we
can include the types of all variables now, the information on whether
a model has integer variables can be determined from that.

Indeed, we update all tests to contain the type information as well.
@fuglede
Copy link
Contributor Author

fuglede commented Oct 9, 2023

It would also be nice to simply read it off the Highs object itself (so as to avoid parsing another file format in a context, where all that's available is a virtual file system in the first place). But, as far as I can tell, that involves adjusting or appending to the C API, as that's all that's available here for now.

For the particular issue with IsLinear and IsQuadratic, I would propose simply getting rid of them altogether: I don't have the imagination to see a scenario where the user doesn't already know if their model has quadratic terms prior to running it, and whether they have integer variables or not (although that can also be read off the "Type" information that is now available in all cases).

@jajhall
Copy link
Collaborator

jajhall commented Oct 9, 2023

@jajhall does the computer-readable format have the same information as the human-readable format in the latest versions ?

It has all the solution information, that's for sure. However, now you ask, I remember the issue of you wanting the bounds on the variables, and getting this from the human-readable format. Is that so?

@jajhall
Copy link
Collaborator

jajhall commented Oct 9, 2023

It would also be nice to simply read it off the Highs object itself (so as to avoid parsing another file format in a context, where all that's available is a virtual file system in the first place). But, as far as I can tell, that involves adjusting or appending to the C API, as that's all that's available here for now.

The solution is immediately available via the C API. Call Highs_getSolution and, where relevant, Highs_getBasis

@lovasoa
Copy link
Owner

lovasoa commented Oct 10, 2023

I'd be ok with removing isLinear and isQuadratic (bumping the version number, because that would not be backwards compatible).

But we still need to throw a js error on error. I saw you removed that test.

tests/test.js Show resolved Hide resolved
tests/test.js Show resolved Hide resolved
@lovasoa
Copy link
Owner

lovasoa commented Oct 10, 2023

We also need to update the README, since we changed the solution format

@fuglede
Copy link
Contributor Author

fuglede commented Oct 10, 2023

We also need to update the README, since we changed the solution format

Should be covered already unless I missed something.

Copy link
Owner

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, let's go

@lovasoa lovasoa merged commit e25a423 into lovasoa:main Oct 10, 2023
1 check passed
@fuglede
Copy link
Contributor Author

fuglede commented Oct 10, 2023

Wonderful, thanks for taking the time to review, @lovasoa!

@lovasoa
Copy link
Owner

lovasoa commented Oct 10, 2023

thanks for your work, @fuglede

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants