Reduce MDD#949
Conversation
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Good, some cleanups and improvements for readibility possible.
Please also add a small tests tests/test_globalconstraints.py to ensure the reduce step is working correctly.
tias
left a comment
There was a problem hiding this comment.
from what I see and realize now, the constructor does too much work...
it should just store (arr, transitions), then self.root_node,
and then it can compute self.sink_node by taking set(id2) - set(id1)?
and then the _get_complete_mdd() would become get_mapping(complete=True) which would create those (mapping, levels) for decompose?
so then the question is reduce, the goal is to rewrite/remove rows from the table, right? can that be done more directly?
If not, then at the least I think you should make sure that reduce keeps self.levels and self.mapping correct too?
|
I don't want to complicate things... but I don't like double information either. I checked the type: 'transitions' does not actually contain variables, it never will, its constants. So we actually don't need to store 'transitions' I think... we can just build self.mapping (and self.levels) and then keep most of the code the same (we just walk over mapping most of the time I assume), and then the reduce code just needs to make sure that removed nodes are removed from self.levels too? |
|
The way I see it is as follows: transitions is only an input argument to the MDD global, and is used to build self.mapping in the constructor. After that, transitions is indeed no longer needed, and it's never used outside of the constructor. self.mapping is the internal representation of the MDD, which changes when calling reduce(). But indeed, removed nodes do need to be removed from self.levels too since that is used outside of the constructor. |
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Great, some final small comments that need to be addressed before merging (I'll already approve otherwise I have to re-review...)
Continuing on #924 and #945, I added an auxiliary function _reduce() to the MDD global constraint that identifies equivalent nodes: in other words, nodes with the same set of completing suffixes.