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

HiGHS: Construct lists before loop #797

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

christiansegercrantz
Copy link
Contributor

@christiansegercrantz christiansegercrantz commented Feb 20, 2025

Initialising the values in the loop and accessing them through the dictionary as done is very slow. This speeds up the operation from 11s/1000 constraints to ~0.5mil in sub 1 second.

Related to issue #793

(Reviewing the CLA with my employer, hold on)

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2025

CLA assistant check
All committers have signed the CLA.

@christiansegercrantz christiansegercrantz marked this pull request as ready for review February 20, 2025 15:20
@christiansegercrantz
Copy link
Contributor Author

I seem to be unable to request reviews, but would @pchtsp be able to review?

@christiansegercrantz christiansegercrantz changed the title Construct lists before loop HiGHS: Construct lists before loop Feb 21, 2025
Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

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

Thanks. It's not clear to me why this approach would be more efficient than the previous one. Do you have any idea?

It seems that solution.row_value, solution.row_dual are already lists. Why would you need to cast them into a list?

Other than that, I don't see any issue with the code itself.

@christiansegercrantz
Copy link
Contributor Author

christiansegercrantz commented Feb 21, 2025

The reason is that you are doing access operations every loop. It's calling the row_value from the solutions object that is slow as far as I understand. As far as I know, it's not recommended have function calls in for statements. Like

for x in function():
    ...

is not great. Do

values = function()
for x in values:
    ...

instead. But this I am not 100% sure about. But the speed here is of multiple magnates. You/I can run a profiler on it if you like to show the diff.

@pchtsp
Copy link
Collaborator

pchtsp commented Feb 21, 2025

iterating over dict.values() is not inefficient. In fact, by casting it into a list, you're already incurring into some (small) additional time.

In any case, I take your word for it.

@pchtsp pchtsp merged commit 6f9138a into coin-or:master Feb 21, 2025
16 checks 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.

3 participants