search

clulab / eidos

Machine reading system for World Modelers
Apache License 2.0
36 stars 24 forks source link

Work on branch #1075

Closed kwalcock closed 2 years ago

kwalcock commented 2 years ago

Call it branchOpt Reuse emptyOntologyGrounding Access branch through IndividualGrounding

kwalcock commented 2 years ago

This makes the branch accessible from IndividualGroundings, I think. It is not supposed to change the functionality, but I'm not sure whether any more or fewer tests are passing.

In making this change, I looked back almost two years to when the branchOpt was added to the OntologyGrounding. This class describes multiple groundings coming from the same version of the ontology, and I think it was assumed that either all the groundings would have the same described branch, or they would be filtered soon thereafter or just before to have them be the same and thus described by the branchOpt variable. Maybe it didn't work out or was forgotten or I'm wrong. It seems like the SRLCompositionalGrounder should provide the expected branch when it calls things like newOntologyGrounding or emptyOntologyGrounding. If the filter was placed there, the filtering probably wouldn't need to be done with filterSlots in the constructor to the PredicateTuple. Even if filtering would need to be done, the slot is right there in branchOpt and doesn't need to be passed in as an argument. However, perhaps well enough should be left alone. @zupon might need to decide it, maybe based on other considerations entirely.

zupon commented 2 years ago

Thanks for this @kwalcock ! It looks like this should work for our purposes. Not sure about redoing/revisiting the filter earlier on with branchOpt. Something to think about for the future...