-
Notifications
You must be signed in to change notification settings - Fork 64
fix: lp-polars api #485
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
fix: lp-polars api #485
Conversation
@FabianHofmann - appreciate your time with looking into this bug fix. We have seen a massive speed up! |
the speed up is really crazy, and it improved over time it seems. I am just wondering, where is the filtering applied to the ordinary lp export? your change indeed seems to fix the issue. however atm I cannot understand why (is that a precision difference between polars and pandas?). if it is really just about the filtering of close zero coeffs couldn't we then just alter the current
|
The numerical filtering isn't required for this to fix the infeasibilities and can be removed if you prefer. I did this to remove numerical noise. What fixes it is the filtering to ensure each constraint has valid coefficients and RHS. We found the I already have a copy of this without the numerical filtering if you would like me to update. |
I understand, thank you for clarifying. Just to finally get me on board, invalid coeffs and rhs means nans? |
Yes - or "null"s in polars speak. I check that every constraint has coefficients that are not null and a sign that is not null. It is assumed that if the sign is not null, the rhs is also not null. A more robust check would be something like: |
I can't add anything, but just wanted to give props to you guys for working on this! |
great, thanks, I need bit more time to review this properly, would be happy about some effort to make this a more minor change (code-wise) as I'm still getting used to the polars syntax. but when happy to trigger a new release once this is in (early next week I would suggest) |
Sure thing - I've pushed some changes to reduce the scope. I've removed the dropping of small values and focussed only on ensuring the constraints are written in the right format. I hope that makes it easier to review and incorporate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great @CharlieFModo, happy to merge this. out of curiosity, did you give #493 a try? don't know if my tagging reached you
@FabianHofmann - I had a look but didn't attempt to implement it. I think if I attempt anything else in the short term it will be the direct api for the xpress solver or other performance improvements to reduce the overhead to and from the solver. Hope that's ok! |
no problem! |
Closes # (if applicable).
#484
Changes proposed in this Pull Request
Makes
lp-polars
API behave the same aslp
. All screenshots below use the same problem.linopy 0.5.6 - io_api="lp-polars"

this branch - io_api="lp-polars"

linopy 0.5.6 - io_api="lp"

Checklist
doc
.doc/release_notes.rst
of the upcoming release is included.