isi-vista / adam

Abduction to Demonstrate an Articulate Machine
MIT License
10 stars 4 forks source link

Implement affordance learner #1120

Closed lichtefeld closed 2 years ago

lichtefeld commented 2 years ago

Closes #1111 and Closes #1114

This implements the affordance learner for the simulated integrated learner. While in theory this could be used in the symbolic learner aside from some minor testing I didn't do any verification that this learner will work with the symbolic system.

lichtefeld commented 2 years ago

Can the affordance learner match its patterns to static scenes? I don't think it can as things stand, and this is something we need for contrastive learning, and probably for other things. I see two possible fixes.

One would be to change match_template inside template_learner so it can handle matching dynamic patterns to non-dynamic graphs. I don't think this will cause problems if all the temporal scopes are the same, but I'm not sure what we want to do for "inherently dynamic" patterns. Would have to think about that more.

I still need to consider this concern more completely. I think the solution will be when going to match the template is converted from temporal scope into a graph without a temporal scope for non-dynamic scenes.

lichtefeld commented 2 years ago

I think we haven't yet resolved the static/dynamic mismatch entirely. Unfortunately I'm not able to reply on those threads from the web UI -- the reply box is gone. We had discussed the solution of converting the graph to a dynamic one before running the affordance learner. This works at test time but causes problems if we do this naively at learning time. Doing this while learning makes the affordance learner happy but causes problems for static learners like the object learner, because they can't learn from dynamic graphs. I think that is where we left the discussion, correct me if I'm wrong. You planned to think more about what to do at learn time?

I think the solution is the patterns should return either temporally scoped or non-temporally scoped based on the perception graph's settings. For now I'm going to naively implement just removing temporal scope from all edges and see if that works. If it doesn't we'll either enforce 'all temporal scopes must be the same' or we'll need some other method of handling it... Hopefully the 'just convert to non-temporal scope' will be sufficient.

Is it still part of the plan to modify other learners' patterns? We discussed modifying patterns at some point (in Slack?) but I don't recall the outcome of that. The diff deleting the "add node to pattern" function reminded me of this.

Oh right! That's why that utility function was there, I just noticed I didn't end up using it so I removed it. I'll restore it and add that logic in.

spigo900 commented 2 years ago

I think the solution is the patterns should return either temporally scoped or non-temporally scoped based on the perception graph's settings.

So is the following interpretation correct? The affordance learner would enrich but not learn from the graph if it is static, and it would add unwrapped edges in that case. When it's dynamic we learn from the graph and we enrich with dynamic edges "as normal."

lichtefeld commented 2 years ago

@spigo900 Yes that is correct.

lichtefeld commented 2 years ago

@spigo900 For 'adding nodes back into pattern graphs' I'm considering the following approach -- After learning affordances, re-enrich the scene. This will potentially cause duplicate AffordanceSemanticNodes to be generated as one round of enrichment has already run at the start of the analysis for this scene. After enriching the scene have the integrated learner perform analysis over the set of AffordanceSemanticNodes generating a request to the object_learner to update it's patterns for a given concept (the alignment of SLOT1 in the AffordanceSemanticNode with the given node that matches to the created affordance categorical node.

Somewhere in this process a check for duplication of a pattern matching node needs to occur so I'm planning on adding that within the object learner and having that portion of code check if an exact pattern node already exists. Given this is a categorical match I can check for an exact node match easily. Thoughts? My biggest concern is that somewhere in this process pattern nodes will get duplicated which isn't a huge deal for subset but may be a larger concern for other learners? Doesn't impact too much of phase 3 but its a consideration.

spigo900 commented 2 years ago

@lichtefeld I think that sounds good. Duplicating nodes in the graph is not ideal, but as long as it only happens after the other learners have run it's not a huge problem especially given the affordance learner runs near the end of learning. And it sounds like you have a solution to prevent duplicating nodes in the patterns -- the object learner checks for duplication when it accepts the update request from the integrated learner. That works.

lichtefeld commented 2 years ago

@spigo900 I'm now passing precommit and tests but I've requested a re-review because I'd like to make sure I integrated the changes from the continuous value matching correctly into the affordance learner.