diprism / fggs

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

Is it okay to change an EdgeLabel's factor? #51

Closed davidweichiang closed 3 years ago

davidweichiang commented 3 years ago

Since an EdgeLabel's factor is not hashed, is it okay to change it? This is currently done both in bin/sum_product.py and examples/make_pcfg.py (#49). More precisely, it changes the factor's _weights attribute.

The above examples are kludges, but at some point we are going to want to train FGGs, and that will certainly involve changing of weights.

darcey commented 3 years ago

Yes, that should be fine.

Here's my understanding of how hashing works in Python when you call d[o] to look up object o in dict d:

As long as the hash never changes, then the object will always be located in the same list inside the dict, so there won't be any issue finding it. And it will still pass the __eq__() check with itself, since it's the same object.

The only place it could cause issues is if you have two EdgeLabels e1 and e2 s.t. e1 == e2 but e1 is not e2. If e1 is in the dict, then you can also look up e2 in the dict. But if you change the weights on e1, then you won't be able to look e2 up in the dict anymore.

Personally, I'm ok with this. If we a global EdgeLabel registry, then it would avoid this problem, but the more I think about it, the more I think this is a feature rather than a bug, since it lets you (for instance) train two copies of the same HMM on different pieces of data and then compare what weights they learn, or something. It would be annoying if the user had to go through and rename all the terminals in the two copies. With the current code, they can just read the same HMM JSON file in twice.

davidweichiang commented 3 years ago

The only place it could cause issues is if you have two EdgeLabels e1 and e2 s.t. e1 == e2 but e1 is not e2. If e1 is in the dict, then you can also look up e2 in the dict. But if you change the weights on e1, then you won't be able to look e2 up in the dict anymore.

Right, that feels weird to me. Actually, there are two things that feel weird to me: (1) what you said above and (2) the fact that we are comparing floats for equality.

One possible solution is that conjunction could require that the terminal labels have names that are disjoint, and thus avoid doing any equality checking between factors. Then I'd say that maybe EdgeLabel.__eq__ shouldn't compare the factors.

However, conjunction really does need to check domains for equality. With domains, comparing floats is not an issue. I'm tempted to say that domains should not be mutable. In that case, I'd have to fix make_pcfg.py, which does currently change domains.

darcey commented 3 years ago

Oh, I hadn't thought about __eq__() comparisons on floats.

I don't like requiring terminal labels to be disjoint, because I can think of lots of cases where you'd want to reuse a terminal across both grammars. For instance, if you have a terminal edge label/factor that says "the value of this tag in the HMM must be N", then it's conceivable that you might want to use that in both the original grammar and the conjunct (especially if the original grammar was formed via conjunction in a previous step).

I'll have to think more about domains being immutable.

davidweichiang commented 3 years ago

Another solution is for FGGs to not have domains and factors at all, and so when you conjoin two FGGs, there are no domains or factors to check. It's only when you want to compute a sum-product that you have to supply mappings from node and edge labels to domains and factors.

This would also possibly help with https://github.com/diprism/compiler/issues/18.

davidweichiang commented 3 years ago

^ I tried this idea out in branch https://github.com/diprism/fgg-implementation/tree/interpretation.

davidweichiang commented 3 years ago

IMO think separating out domains/factors is the way to go; I'll make it into a PR for further discussion.

davidweichiang commented 3 years ago

Another solution:

darcey commented 3 years ago

I think I like your current solution better. You've convinced me that it makes sense to separate HRG and FGG into different classes.