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

Methods to directly add rows and cols to highs model #9

Merged
merged 6 commits into from
Nov 26, 2023

Conversation

mmghannam
Copy link
Contributor

No description provided.

Copy link
Contributor

@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.

looks good. Two small things: can we make it very obvious in the doc comments which methods can panic, and under which circumstances ?

Can we add asserts to the tests that check that the right values were added ? And also check the error cases of the try_* functions.

@mmghannam
Copy link
Contributor Author

mmghannam commented Nov 26, 2023

looks good. Two small things: can we make it very obvious in the doc comments which methods can panic, and under which circumstances ?

I added a panics section to the docs of the methods that can panic.
The circumstances that they can panic are not in the documentation, I took a look at the highs code; it seems there are many cases but I think some checks are irrelevant here because of the way we call the underlying highs function.

Can we add asserts to the tests that check that the right values were added ? And also check the error cases of the try_* functions.

I'm not sure how to do that without implementing get_cols() and get_rows() methods too. But I added an extra assert for the solution value.

@lovasoa
Copy link
Contributor

lovasoa commented Nov 26, 2023

Godd for me, thanks @mmghannam !

@lovasoa lovasoa merged commit 31ba3f3 into rust-or:main Nov 26, 2023
1 check passed
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.

2 participants