-
-
Notifications
You must be signed in to change notification settings - Fork 401
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 write_to_file and read_from_file #2114
Conversation
My only concern is that we're not following the standard Julia API by allowing reading and writing from an IO object. It's inevitable that we'll get requests for this. Does MOF support writing to streams? |
Looks good to me (even though the failures on Windows are quite strange). Maybe refactor a bit the test suite to avoid having three times the exact same model? (Sorry for the delay, I still have the tab for my PR opened, but had too little time to work on it, unfortunately :(.) I think this API is already quite good as it is. For streams, I suspect Another though: wouldn't the order format-compression for the arguments be easier to grasp? The file names almost invariably have first the file format, then the compression format (like .lp.gz). Considering documentation, copying parts of MOF here really makes sense from a user point of view. However, if a format is added to MOF, it will require a manual modification of JuMP's doc: isn't there any other solution, like including parts of MOF docs from JuMP? |
Codecov Report
@@ Coverage Diff @@
## master #2114 +/- ##
==========================================
+ Coverage 91.01% 91.08% +0.07%
==========================================
Files 40 41 +1
Lines 4350 4376 +26
==========================================
+ Hits 3959 3986 +27
+ Misses 391 390 -1
Continue to review full report at Codecov.
|
I've added the read/write to streams.
Longer term, we should probably pull MOF into MOI. Our longer term goal is to unify the JuMP/MOI documentation in a nice way. |
Nice to hear!
Why aren't you using the "standard" way of defining IO operations on streams (with read and write methods)? This would also imply to have the IO stream as a first argument, but would be more convenient for people picking up JuMP after Julia. |
Mainly to keep consistency with |
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 don't see the point of keeping consistency with write_to_file
for the IO
version. An IO
isn't necessarily a file, so the name is misleading. I prefer using read
and write
with the argument order that's standard in Julia (https://docs.julialang.org/en/v1/manual/style-guide/#Write-functions-with-argument-ordering-similar-to-Julia-Base-1).
I'm making changes to MathOptFormat.jl which should simplify JuMP's implementation. See: odow/MathOptFormat.jl#100. |
I've merged and released the changes to MathOptFormat.jl, so now things are now considerably simpler on the JuMP side. |
Closes #1075
Closes #1982
cc @dourouc05