Skip to content
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 save_as_dot for logic circuits #6

Merged
merged 2 commits into from
May 24, 2020
Merged

Conversation

RenatoGeh
Copy link
Contributor

The function save_as_dot is defined in src/IO/IO.jl, but not implemented. This patch simply adapts the equivalent function save_as_dot defined in ProbabilisticCircuits (https://github.com/Juice-jl/ProbabilisticCircuits.jl/blob/master/src/IO/CircuitSaver.jl) but for LogicCircuits.

Obviously this isn't the best approach to solving this issue, as ideally we should have these two save_as_dot functions call some other more general function. But this would require changing how nodes are printed (perhaps adding a printdot method for each node and simply calling it instead of having two different save_as_dot functions with a bunch of if and elses?) or changing how nodes are typed (in the sense disjunction and conjunction nodes would have a common ancestor, e.g. \bigveeNode and Prob\bigveeNode would be subtypes of DisjunctionNode or whatever).

There is another flaw to this implementation as well: structured nodes are unsupported. Since I'm only working (so far) with the regular Logical\DeltaNodes, and I'm not so familiar with this library yet, I'm choosing to keep it the way it is for now.

Added a test case for the function as well. Not sure the right way of doing it, so will be happy to fix if needed.

@codecov-commenter
Copy link

Codecov Report

Merging #6 into master will increase coverage by 0.82%.
The diff coverage is 92.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   60.82%   61.64%   +0.82%     
==========================================
  Files          26       26              
  Lines        1889     1940      +51     
==========================================
+ Hits         1149     1196      +47     
- Misses        740      744       +4     
Impacted Files Coverage Δ
src/IO/CircuitSaver.jl 89.41% <92.15%> (+4.11%) ⬆️

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 10a1de9...c83bbf6. Read the comment docs.

@khosravipasha khosravipasha self-requested a review May 23, 2020 08:51
@khosravipasha
Copy link
Collaborator

The code looks good to me, made few small changes. Will wait for the tests.

Yeah, it makes lots of sense not to want duplicate code, so I made the get_nodes_level more generalized to take any circuit type (\Delta), and then later on probably can delete it from ProbCircuits repo.

Also we have a node2dag helper to get from a root node to array of nodes, so I replaced that part of code with it, if that was not the indented behavior let me know I can change it back.

@khosravipasha khosravipasha mentioned this pull request May 23, 2020
@khosravipasha khosravipasha added the enhancement New feature or request label May 23, 2020
@khosravipasha khosravipasha merged commit 9f59c92 into Tractables:master May 24, 2020
@RenatoGeh RenatoGeh deleted the dot branch June 24, 2020 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants