april-tools / cirkit

a python framework to build, learn and reason about probabilistic circuits and tensor networks
https://cirkit-docs.readthedocs.io/en/latest/
GNU General Public License v3.0
71 stars 1 forks source link

QuadTree in new is broken #195

Closed lkct closed 4 months ago

lkct commented 8 months ago

found by cc @LeanderK

this is currently not covered by main branch unit tests

AssertionError                            Traceback (most recent call last)
Cell In[6], line 2
      1 from cirkit.new.region_graph import QuadTree
----> 2 region_graph = QuadTree(shape=(width, height), struct_decomp=False)

File /disk/scratch/lkurscheidt/cirkit/cirkit/new/region_graph/algorithms/quad_tree.py:139, in QuadTree(shape, struct_decomp)
    137     node = _merge_4_regions_struct_decomp(graph, regions)
    138 elif len(regions) == 4 and not struct_decomp:
--> 139     node = _merge_4_regions_mixed(graph, regions)
    140 else:
    141     # NOTE: In the above if/elif, we made all conditions explicit to make it more
    142     #       readable and also easier for static analysis inside the blocks. Yet the
    143     #       completeness cannot be inferred and is only guaranteed by larger picture.
    144     #       Also, should anything really go wrong, we will hit this guard statement
    145     #       instead of going into a wrong branch.
    146     assert False, "This should not happen."

File /disk/scratch/lkurscheidt/cirkit/cirkit/new/region_graph/algorithms/quad_tree.py:75, in _merge_4_regions_mixed(graph, region_nodes)
     73 # Merge vertically then horizontally.
     74 region_lft = _merge_2_regions(graph, region_nodes[0::2])
---> 75 region_rit = _merge_2_regions(graph, region_nodes[1::2])
     76 # Reuse the region_whole that is already constructed.
     77 graph.add_partitioning(region_whole, (region_lft, region_rit))
...
     85 self.add_node(head)
---> 86 assert tail.outputs.append(head), "The edges in RG should not be repeated."
     87 head.inputs.append(tail)

AssertionError: The edges in RG should not be repeated.
lkct commented 8 months ago

bug at ln.130. should be if node != PADDING https://github.com/april-tools/cirkit/blob/9ab64f28f41b7e42a866d85eaae15ce81db758b2/cirkit/new/region_graph/algorithms/quad_tree.py#L122-L131

We're looking into designing tests and this issue will be kept open for now.

loreloc commented 7 months ago

Fixed in new, but we need unit tests.