diprism / fggs

Factor Graph Grammars in Python
MIT License
13 stars 3 forks source link

Simplify usage of interpretations / make FGG into subclass of HRG #142

Closed davidweichiang closed 2 years ago

davidweichiang commented 2 years ago

Make some changes that simplify the way that interpretations are created and used:

Maybe it can be simplified more but maybe it depends on how #139 turns out.

darcey commented 2 years ago

Based on your comment about #139, do you want me to hold off on reviewing this for the moment?

davidweichiang commented 2 years ago

The further simplification I had in mind could wait for a later PR, but I thought it is still confusing/complicated for the user to have to write fgg.interp.factors instead of just fgg.factors (similarly for fgg.interp.domains and fgg.interp.shape()).

The options I had thought of were:

  1. Add forwarding properties FGG.domains and FGG.factors and a forwarding method FGG.shape.

  2. Change Interpretation into InterpretationMixin, and make both FGG and FactorGraph inherit from it.

davidweichiang commented 2 years ago

Since #139 was okay, maybe I'll give (2) a try tomorrow.

davidweichiang commented 2 years ago

OK, this is ready for review. I tried making Interpretation into another mixin, and then it seemed like the right thing to do would be to just make FGG a subclass of HRG (and FactorGraph a subclass of Graph). I don't remember if it used to be that way at one time; I do remember wanting a strict separation between HRGs and FGGs and totally can't remember why I wanted it. But subclassing feels right now, and examples/parser/parser.py is looking a lot better (about half the complaints in #140 are addressed here).

davidweichiang commented 2 years ago

@darcey thanks for the review; does it look okay now?