search

clulab / eidos

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

attempt at simplifying #1056

Closed BeckySharp closed 3 years ago

BeckySharp commented 3 years ago

@kwalcock thoughts? There were 2 methods in the superclass(es) that both seemed to (a) receive text and (b) return an OntologyGrounding, but where one didn't call the other. In the subclass SRLOntologyGrounding at least I don't think the right one was overridden. Basically, there was confusion. Here I removed one and searched for any stray usages to remove them.

kwalcock commented 3 years ago

One of the entries about has an unacceptable time stamp. You've been warned.

BeckySharp commented 3 years ago

One of the entries about has an unacceptable time stamp. You've been warned.

:) I’ve been having trouble sleeping lately, I seem to wake up between 3:30 and 4:30 and can’t go back to sleep. I guess anxiety but who knows… hopefully will resolve soon!

kwalcock commented 3 years ago

Is this isGroundableType: Boolean ever anything but true? I don't offhand see it and the compiler agrees. However, it may be called from some client of Eidos. Do you recall why it exists?

BeckySharp commented 3 years ago

Is this isGroundableType: Boolean ever anything but true? I don't offhand see it and the compiler agrees. However, it may be called from some client of Eidos. Do you recall why it exists?

I think it exists bc we pass all mentions through the ground method, but only some are eligible for grounding (i.e., Concepts are but Causal aren't).  I don't think any of the grounders use their own implementation, instead I think they all use this but I could be wrong. Or am I wrong and that method is no longer being called?

kwalcock commented 3 years ago

Each of the grounders, when grounding Mentions, seems to call that method groundableType() on its own and act accordingly. It's important when the mention is around, but if we're down to just text then the decision seems to already have been made to proceed. It seems safe to simplify it away if you don't mind me adding/removing it.

BeckySharp commented 3 years ago

Each of the grounders, when grounding Mentions, seems to call that method groundableType() on its own and act accordingly. It's important when the mention is around, but if we're down to just text then the decision seems to already have been made to proceed. It seems safe to simplify it away if you don't mind me adding/removing it.

gotcha, yes, makes sense! BTW -- is it because EidosOntologyGrounder is abstract that it doesn't have to define groundEidosMention from the trait?

kwalcock commented 3 years ago

I would think so.