-
Notifications
You must be signed in to change notification settings - Fork 7
Update to lrslib v7.1a #41
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
Conversation
d160a90 to
f737302
Compare
| NEs = NTuple{2,Vector{Rational{BigInt}}}[] | ||
|
|
||
| # Step 1 | ||
| FirstTime = cglobal((:FirstTime, liblrsnash), Clong) |
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.
@oyamad Do you know what we should do in lrslib v7.1a for this ? With this line, it complains that FirstTime is not defined and without this line, I have a test failing (see the test logs, not sure if that's related to this line or if that's another change).
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.
Yes, I believe we still need it: there is no change in v7.1a: https://github.com/JuliaPolyhedra/lrslib/blob/ab1a5313a54a6d9aadcaa2f2e8523856f747f7e1/lrsnashlib.c#L55
Isn't this as pointed out by @bzinberg in JuliaPolyhedra/lrslib#2 (comment)?
The declaration of FirstTime has been removed in lrsnashlib.h in v7.1a: JuliaPolyhedra/lrslib@607673e#diff-8325ef3172d6193ffabfcb846d744f86c389a21a61c802c4c715f55462857204L70
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.
Agreed. In case helpful, my thoughts as a newcomer to this codebase are:
- Changing the public API from an
extern longthat the user unsafe-sets to areset()method is a good start. But probably more important, the re-implementation oflrs_solve_nashin Julia here seems like an anti-pattern to me. It indicates thatFirstTimereally isn't a public API, it's just needed to enable the implementation to be split across the C codebase and the Julia codebase. This is doable if it's needed, but perhaps we can revisit whether there's a way to bring all the logic into C with a documented API? FirstTimeappears to be global state for the Nash solver. I wonder if that state could be factored into a struct so that the logic in lrsnashlib could become stateless?- (I'm not sure why we're using a
longfor what apparently has the semantics of a boolean, but I assume there is a good reason for that.)
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.
I agree. If the API was sufficient, we shouldn't have to define the structs in Julia. However, it's sometimes difficult to bring the API to that level to the upstream package.
We already contacted David Avis trying to merge the changes we need upstream but we probably complicated things asking to have the build process work on more platforms which is now resolved and simplified thanks to the cross-compilation with Yggdrasil. We could aim at getting
JuliaPolyhedra/lrslib@3962f83
JuliaPolyhedra/lrslib@ab1a531
and
JuliaPolyhedra/lrslib#3
to the next lrslib release and we could get rid of the fork.
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 83.70% 83.85% +0.15%
==========================================
Files 8 8
Lines 632 638 +6
==========================================
+ Hits 529 535 +6
Misses 103 103
Continue to review full report at Codecov.
|
No description provided.