Currently, when we update a hypotheses we intersect it with multiple candidate hypotheses from the observed scene. There is one intersection per "thing" in the scene. So, for objects we have one hypothesis per detected object. This wasn't a problem previously because the pattern nodes were immutable. But @sidharth-sundar noticed a problem while implementing the color matching improvements (#1128): Every successful intersection triggers a "confirm match" update on the same pattern nodes.
This poses two problems for mutable nodes (specifically the improved color node #1128 and the improved continuous feature node #1119). First, because of an argument ordering issue we end up updating the hypotheses that were created from the perception, rather than the previous hypotheses. Second, because
The first issue is annoying. It seems oddly backwards and results in some weird shouldn't-happen updates, resulting in a crash. Fortunately, this is easy to patch (see the quick patch below). However, the other issue remains.
(This crash does show up during training on the M5 objects because in some samples the GNN detects more than one object even though there's exactly one. In fact that problem is why I had to set beam size to 1 in the objects experiment.)
Fixing the problem
Ideally, we might rework the update logic to work with immutable nodes. I think I see a way to do that (see section below), but it seems more annoying than I want to try right now. Instead, I propose we work with what we have.
I think we should implement the quick patch, and also deepcopy (from copy import deepcopy) the previous hypothesis before updating it here. So we'd be passing previous_pattern_hypothesis.copy() instead of the bare hypothesis. This copying should prevent us doing multiple updates to the same node, because now we're operating on a copy of the node.
(Unfortunately, as far as my thinking goes it looks like we are going to end up doing a lot of copying whether we go immutable or not. The immutable might be slightly more performant but requires more changes to the parts of the node now trying to mutate things... 🙃)
PS: Quick patch, plus the weird thing that happens
Specifically the weird thing that can happen is that we try to combine two distributions with multiple observations -- the previous hypothesis which should have multiple, and the multiply-updated hyopthesis-from-observation which should not. This leads to a crash because we don't know how to do a multi vs. multi update. In the multi vs. single case we can treat the single-obs distribution as just a single observation. In the multi-obs case we don't know how to combine them and have to crash.
PS: The annoying immutable way of updating
Replace the current scheme "functional" methods. Replace the confirm_match() family of methods on PatternMatch/PerceptionGraphTemplateIntersectionResult with ones that return new graphs. Replace the confirm_match() method on pattern nodes with one that returns a new pattern node. Rewrite the existing implementations accordingly.
This would probably cause performance problems, but I think if we did this it would still worth implementing the naive approach. That way we can profile to see where the problems are. I expect the copying would be a problem. If that's the case we may want to store on each pattern a flag for "has continuous features" (really, just a flag saying that it need to be updated via this process) and only update this way when that flag is set, to avoid the copying. But again, this would be something to check via profiling before we go complicating the pattern graph implementation further.
Currently, when we update a hypotheses we intersect it with multiple candidate hypotheses from the observed scene. There is one intersection per "thing" in the scene. So, for objects we have one hypothesis per detected object. This wasn't a problem previously because the pattern nodes were immutable. But @sidharth-sundar noticed a problem while implementing the color matching improvements (#1128): Every successful intersection triggers a "confirm match" update on the same pattern nodes.
This poses two problems for mutable nodes (specifically the improved color node #1128 and the improved continuous feature node #1119). First, because of an argument ordering issue we end up updating the hypotheses that were created from the perception, rather than the previous hypotheses. Second, because
The first issue is annoying. It seems oddly backwards and results in some weird shouldn't-happen updates, resulting in a crash. Fortunately, this is easy to patch (see the quick patch below). However, the other issue remains.
(This crash does show up during training on the M5 objects because in some samples the GNN detects more than one object even though there's exactly one. In fact that problem is why I had to set beam size to 1 in the objects experiment.)
Fixing the problem
Ideally, we might rework the update logic to work with immutable nodes. I think I see a way to do that (see section below), but it seems more annoying than I want to try right now. Instead, I propose we work with what we have.
I think we should implement the quick patch, and also deepcopy (
from copy import deepcopy) the previous hypothesis before updating it here. So we'd be passingprevious_pattern_hypothesis.copy()instead of the bare hypothesis. This copying should prevent us doing multiple updates to the same node, because now we're operating on a copy of the node.(Unfortunately, as far as my thinking goes it looks like we are going to end up doing a lot of copying whether we go immutable or not. The immutable might be slightly more performant but requires more changes to the parts of the node now trying to mutate things... 🙃)
PS: Quick patch, plus the weird thing that happens
Specifically the weird thing that can happen is that we try to combine two distributions with multiple observations -- the previous hypothesis which should have multiple, and the multiply-updated hyopthesis-from-observation which should not. This leads to a crash because we don't know how to do a multi vs. multi update. In the multi vs. single case we can treat the single-obs distribution as just a single observation. In the multi-obs case we don't know how to combine them and have to crash.
PS: The annoying immutable way of updating
Replace the current scheme "functional" methods. Replace the
confirm_match()family of methods on PatternMatch/PerceptionGraphTemplateIntersectionResult with ones that return new graphs. Replace theconfirm_match()method on pattern nodes with one that returns a new pattern node. Rewrite the existing implementations accordingly.This would probably cause performance problems, but I think if we did this it would still worth implementing the naive approach. That way we can profile to see where the problems are. I expect the copying would be a problem. If that's the case we may want to store on each pattern a flag for "has continuous features" (really, just a flag saying that it need to be updated via this process) and only update this way when that flag is set, to avoid the copying. But again, this would be something to check via profiling before we go complicating the pattern graph implementation further.