-
Notifications
You must be signed in to change notification settings - Fork 7
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
Update to lrslib v7.1a #41
Conversation
d160a90
to
f737302
Compare
@@ -46,10 +46,6 @@ end | |||
function solve_nash(hr1::HMatrix, hr2::HMatrix) | |||
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 long
that the user unsafe-sets to areset()
method is a good start. But probably more important, the re-implementation oflrs_solve_nash
in Julia here seems like an anti-pattern to me. It indicates thatFirstTime
really 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? FirstTime
appears 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
long
for 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 struct
s 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.