Tractables / LogicCircuits.jl

Logic Circuits from the Juice library
https://tractables.github.io/LogicCircuits.jl/dev/
Apache License 2.0
48 stars 4 forks source link

Create nonexisting literals when smooth_node does not find them #108

Closed RenatoGeh closed 2 years ago

RenatoGeh commented 2 years ago

Hi,

Here's a fix to https://github.com/Juice-jl/ProbabilisticCircuits.jl/issues/80.

The issue was that smooth_node would only account for existing literal nodes. This means that, in a logic circuit like this one

a

smoothing with smooth_node won't be able to find neither literal 2 nor 4 on line

target_vtr = lca(map(x -> vtree(lit_nodes[x]), collect(parent_scope))...)

because lit_nodes only covers seen literals.

Here's a minimal example of this issue as covered in https://github.com/Juice-jl/ProbabilisticCircuits.jl/issues/80, but here adapted to the new API:

using LogicCircuits, ProbabilisticCircuits, Random
Random.seed!(1)
sdd = compile(SddMgr(Vtree(4, :random)), read("/tmp/test.cnf", LogicCircuit, FnfFormat()))
psdd = compile(StructProbCircuit, sdd)

This gives the same error as mentioned in the original issue. Note that this only happens with the StructLogicCircuit version of smooth_node, since the other, more general variant, already takes into account when we can't find the non-existing literal.

This PR simply adds a condition that, if the literal has not yet been seen by lit_nodes, then create a new one.

Let me know if and how you want me to add test cases.

Thanks, Renato

codecov[bot] commented 2 years ago

Codecov Report

Merging #108 (0c38c29) into master (54f9eb3) will increase coverage by 0.01%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   85.49%   85.51%   +0.01%     
==========================================
  Files          25       25              
  Lines        2737     2740       +3     
==========================================
+ Hits         2340     2343       +3     
  Misses        397      397              
Impacted Files Coverage Δ
src/transformations.jl 96.09% <100.00%> (+0.03%) :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 54f9eb3...0c38c29. Read the comment docs.

khosravipasha commented 2 years ago

Thanks for the PR. I see, this makes sense overall. I will take a closer look, but yeah having this minimal example as test would be great.

RenatoGeh commented 2 years ago

Thanks for the PR. I see, this makes sense overall. I will take a closer look, but yeah having this minimal example as test would be great.

Ok, done! Thanks :)