-
Notifications
You must be signed in to change notification settings - Fork 4
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
JOSS Review Comments #50
Comments
Hi @elsoroka could you please have a look. |
Hi! yes, so sorry this has been delayed. I am working at an internship over the summer and have limited time -- we get a couple days off for the holiday this week so I'm targeting end of the week to address these comments. |
Addressing comments: Documentation
Paper
Nitpicks
|
@elsoroka I tried the following for CVC4 and similar for Alt-Ergo: using Satisfiability
@satvariable(p, Bool)
@satvariable(q, Bool)
@satvariable(r, Bool)
conjecture = ((p ⟹ q) ∧ (q ⟹ r)) ⟹ (p ⟹ r)
status = sat!(!conjecture, solver=CVC4())
println(status) Receiving:
As for everything else, the only thing remaining that I haven't seen is a clear list of dependencies. Once that is resolved and I hear a response from this comment, I think I'll be satisfied with the submission. Almost everything is checked on my checklist. |
Not a fix yet: I tested this for CVC4 and I can see two things: I'm looking into why this is and will respond with an update. |
Explanation: CVC4 prints input on its output!First, a command that should work for launching CVC4 is MWE of something that should work:using Satisfiability
# really simple problem
@satvariable(a, Bool)
@satvariable(b, Bool)
conjecture = iff(a ∧ b, ¬(¬a ∨ ¬b))
print(smt(conjecture)) # just to check
# Define a new Solver object
# https://elsoroka.github.io/Satisfiability.jl/dev/advanced/#Custom-solver-options-and-using-other-solvers
CVC4() = Solver("CVC4", `cvc4 --lang smt2 --interactive --incremental --produce-models --no-interactive-prompt -q`)
# This should work, but it does not.
# Note that the logic is set because CVC4 requires it, while Z3 and CVC5 don't.
sat!(conjecture, solver=CVC4(), logic="ALL") When we do this, we get the error
Now, "Solver error" is printing the output Satisfiability.jl got from CVC4. Internally, this is what sat! does:
So this almost looks right. But the problem is... CVC4 echoes the input (stdin) to the output (stdout)That's why the INPUT lines (set-logic), (declare-fun) etc. appear in the error. This is really sneaky because to our eyes, this is right. But to the parser, which was expecting either Here's a comparison with Z3: Demonstration: finding our input on CVC4's stdout# Now we can open an interactive process to inspect what's going on
begin
isolver = open(CVC4())
send_command(isolver, "(set-logic ALL)", dont_wait=true)
assert!(isolver, conjecture)
sleep(0.02)
println("Read from pstdout:")
__stdout = readavailable(isolver.pstdout)
print(String(__stdout))
# Instead of waiting for sat, we'll leave the output in the pipe and print it manually
send_command(isolver, "(check-sat)", dont_wait=true)
println("\nFinished sending commands.")
println("Now pstdout should read SAT")
sleep(0.02)
__stdout = readavailable(isolver.pstdout)
print(String(__stdout))
close(isolver)
end Output:
Demonstration: Z3 doesn't do this# Now we can open an interactive process
# this will allow us to inspect what's going on
begin
isolver_2 = open(Z3())
assert!(isolver_2, conjecture)
# We don't read here because readavailable() will hang
# there are NO byptes in pstdout because Z3 does not have this behavior
# Instead of waiting for sat, we'll leave the output in the pipe and print it manually
send_command(isolver_2, "(check-sat)", dont_wait=true)
println("Finished sending commands.")
println("Now pstdout should read SAT")
sleep(0.02)
__stdout2 = readavailable(isolver_2.pstdout)
print(String(__stdout2))
close(isolver_2)
end Output:
@computablee what are your thoughts on this? Are you aware of a command-line option for cvc4 that will stop the input from being echoed to the output? If so, that would fix the problem. |
@elsoroka I'm not aware of such an option. There is a Python API and a C++ API for CVC4, but I realize implementing either of those is asking a lot when CVC5 is already supported and better in almost all ways in my experience. I know the Why3 platform can interface with both CVC4 and CVC5... it might be worth checking how they do it. In my opinion, just knowing that CVC4 is not supported is sufficient. I wasn't aware of the |
Does Alt-Ergo workI'm actually not sure how to test this. The reason is Satisfiability.jl relies on opening an interactive session, so we can do things like decide whether to say This has overall been helpful because now we have more examples to discuss in the documentation. And I will make a Git issue about CVC4 and your suggestions for other packages that work with it @computablee. DependenciesThey are in the Project.toml file https://github.com/elsoroka/Satisfiability.jl/blob/main/Project.toml |
@computablee are you ready to mark this thread solved? |
@computablee could you please have a look? |
So sorry about the late response! Work has been busy. Yes, I think my issues have been solved. I'll mark it on the review thread. Would you like me to close this issue? |
I think since the review thread is solved @computablee, this one can be closed. Thank you! |
Sounds good! Closing @elsoroka |
Hello! First, I want to mention that it has been a pleasure to see some SMT work being submitted to JOSS. I have checked almost all of the boxes on my review, but there are just a couple of things that I think could be improved. I'll go over each major section of my checklist and explain why I've checked what I did, and why I haven't checked what I didn't.
From the JOSS submission.
Functionality
Documentation
CONTRIBUTING.md
for contributing to the repository. I also don't see aCODE_OF_CONDUCT.md
or issue templates. Please consider adding these.Software Paper
Nitpicks
(p ⟹ q) ∧ (q ⟹ r)
is true if and only if(p ⟹ r)
, not whether the former implies the latter as referenced by the text. Changing the code we verify that the example given does indeed hold:v0.1.2
out, but this is not reflected in other parts of the documentation like the site.Conclusion
Overall, I think a lot of the things here are minor and easily fixable. I don't think there's any glaring issues with the submission, so I cast my review as an acceptance with minor revisions. Thank you again for your submission!
The text was updated successfully, but these errors were encountered: