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

Check 1452 #1454

Closed
wants to merge 4 commits into from
Closed

Check 1452 #1454

wants to merge 4 commits into from

Conversation

jajhall
Copy link
Member

@jajhall jajhall commented Oct 10, 2023

So, @fwesselm, an assert after

// there are many collisions, determine the exact sizes first

is triggered by check/TestHighsHash.cpp, but not in any of the MIP instance tests

@fwesselm
Copy link
Contributor

Interesting, I cannot reproduce this on Windows. Could you tell me which assert is triggered? Thank you.

@jajhall
Copy link
Member Author

jajhall commented Oct 11, 2023

The assert that I added is not yet pushed to GitHub - and it probably won't. I've run the easiest 100 miplib problems (in release, with a print statement) to see how often it's used in reality.

It's not that I don't trust what you've done: I'm just curious to see why an apparent bug hasn't caused more trouble, and what that may tell us about the error in the user's issue

Comment on lines +658 to +660
default:
throw std::logic_error(
"Unexpected result from entries_to_size_class");
Copy link
Contributor

Choose a reason for hiding this comment

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

Note the discussion on exceptions here: #1368

The basic takeaway is that since they require catching and handling before being passed to the C API they might not be of much use without a more principled effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the very least it should be caught and rethrown to ensure correct propagation.

@fwesselm
Copy link
Contributor

The assert that I added is not yet pushed to GitHub - and it probably won't. I've run the easiest 100 miplib problems (in release, with a print statement) to see how often it's used in reality.

It's not that I don't trust what you've done: I'm just curious to see why an apparent bug hasn't caused more trouble, and what that may tell us about the error in the user's issue

FYI, the new line (correctly zero out array) is hit on problem fast0507 on Windows for example.

@fwesselm
Copy link
Contributor

I created #1470 to fix #1372. Therefore, this PR could be closed, I think.

@jajhall jajhall closed this Oct 25, 2023
@jajhall jajhall deleted the check-1452 branch November 3, 2023 11:06
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.

None yet

3 participants