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

add primalColumnSolution and dualColumnSolution properties #120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PaulFlanaganGenscape
Copy link

For improved performance, adding 2 new properties

  1. primalColumnSolution as alternative to 'primalVariableSolution'
  2. dualColumnSolution as alternative to 'dualVariableSolution'

These 2 new properties allow solution data to be retrieved as single numpy array, instead of the dictionary format provided by primalVariableSolution and dualVariableSolution.

This provides better performance when solution as numpy array is all that is required.

@CLAassistant
Copy link

CLAassistant commented Apr 14, 2021

CLA assistant check
All committers have signed the CLA.

@PaulFlanaganGenscape
Copy link
Author

Further details:
The small models, the properties primalVariableSolution and dualVariableSolution can take significant time to return data.

Below is example output showing the overhead of calling primalVariableSolution and dualVariableSolution
If I could use primalColumnSolution and dualColumnSolution instead, that overhead of 13.9 seconds would disappear

0.322 seconds to add 364560 vars to model
0.005 seconds to add 7440 constraints to model
0.018 seconds to add objective to model
0.766 seconds to solve model
13.903 seconds to get solution using primalVariableSolution and dualVariableSolution

Do I need to do anything else for this PR to be accepted?

@tkralphs
Copy link
Member

You had said you would add some tests, but I'm sure you noticed that the existing tests are not that well-maintained and I haven't taken the time to set up automated testing yet (on the list). This is a side project that I don't have a lot of bandwidth for. But if you could add a test or at least paste in a script I could run for minimal verification here, I would try to build this PR and ensure everything looks OK.

@PaulFlanaganGenscape
Copy link
Author

@tkralphs I have added a test for the new properties

Thank you very much for the work you do on this project. It's very much appreciated

@PaulFlanaganGenscape
Copy link
Author

@tkralphs Do you require anything else for this PR ?

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