chkwon / PATHSolver.jl

provides a Julia wrapper for the PATH Solver for solving mixed complementarity problems
http://pages.cs.wisc.edu/%7Eferris/path.html
MIT License
50 stars 15 forks source link

Enable output for Jupyter scenarios #49

Closed davidanthoff closed 3 years ago

davidanthoff commented 3 years ago

The idea here is simply to pass a mutable struct instead of the IO in the output_data field, because we then don't run into the problem that we can't pass an immutable there.

I tested this on Windows in the REPL and in Juptyer Lab. There is one very weird thing that makes me kind of nervous, I'll comment on that specific line.

odow commented 3 years ago

I'll merge https://github.com/chkwon/PATHSolver.jl/pull/51 then restart CI.

codecov[bot] commented 3 years ago

Codecov Report

Merging #49 (9d0d2d2) into master (d26861d) will increase coverage by 0.72%. The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   92.07%   92.80%   +0.72%     
==========================================
  Files           3        3              
  Lines         265      264       -1     
==========================================
+ Hits          244      245       +1     
+ Misses         21       19       -2     
Impacted Files Coverage Δ
src/C_API.jl 90.24% <77.77%> (+1.15%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d26861d...9d0d2d2. Read the comment docs.

odow commented 3 years ago

Looks like a segfault on Mac now. What if you try wrapping stdout in the mutable struct?

davidanthoff commented 3 years ago

Yes, let me try that! Maybe the problem is that my current design turns the _c_flush and _c_print into closures over global variables, which your suggestion would avoid.

davidanthoff commented 3 years ago

Alright, locally tests pass and I don't see any segfaults, even without that weird flush call, so this seems much better. Lets see whether CI passes :)

davidanthoff commented 3 years ago

@odow or @chkwon who would merge this one and then tag a new version? Anything else I should do on this PR? I think in terms of tests we should be good because this essentially just simplifies existing functionality that is presumably already covered by tests, right?

davidanthoff commented 3 years ago

Thanks!