Fix combinatorial blowup in ConstraintGraphFactory._recurse() from missing negative-memoization - #177
Open
thanhndv212 wants to merge 1 commit into
Conversation
…issing negative-memoization GraphFactoryAbstract._recurse()'s isNewState check was `not self._existState(nGrasps)`, but self.states (backing _existState) is only populated when graspIsAllowed(nGrasps) is True. So a *rejected* grasp combination was never marked visited, and got its entire descendant subtree redundantly re-explored from every distinct parent path that reached it. The subtree rooted at a given nGrasps is a pure function of its content alone (ngrippers/nhandles are the input lists with a fixed set removed, independent of removal order; depth is likewise path-independent), so re-reaching the same nGrasps via a different gripper-assignment order is always pure wasted recomputation, never new discovery. Add a separate _visitedGrasps set, populated for every nGrasps regardless of accept/reject, independent of self.states (which has a different meaning: actually-created State objects that other code depends on). For 8 grippers x 7 handles (a downstream project's worst-case scene), this caused a 20+ minute hang building the constraint graph for a single phase. Also corrects the class docstring, which described recursion stopping on a rejected grasp set — it doesn't and shouldn't (needed to support non-monotonic filter rules); the docstring just didn't match the code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GraphFactoryAbstract._recurse()memoizes visited grasp combinationsvia
self.states/_existState(), butself.statesis only populatedfor combinations accepted by
graspIsAllowed. Rejected combinationswere never marked visited, so their entire descendant subtree was
redundantly re-explored from every distinct parent path that reached
them.
Downstream, this caused a 20+ minute hang building the constraint
graph for a phase with 8 simultaneously-held grippers / 7 objects — a
case with a restrictive filter that should have been fast per this
module's own docstring (O(N) for such filters), but wasn't, because
rejections were the majority case and none of them were cached.
Fix
Add a
_visitedGraspsset, separate fromself.states(real createdState objects, used elsewhere), populated for every
nGraspstupleregardless of whether
graspIsAllowedaccepted or rejected it.Testing
Standalone, HPP-independent reproduction (stubs the two compiled deps
_recursedoesn't actually touch —pyhpp.constraints.{Implicit, LockedJoint},numpy; a trivial concrete subclass recording intoplain sets instead of touching real graph objects):
{grasps}state-set and{(from,to,ig)}transition-set between unpatched and patched
_recursefor N=2..8grippers/handles, including an adversarial non-monotonic filter
(rejects most intermediate combinations, accepts one specific deep
target) — states/transitions match exactly in every case; only the
redundant recursion is eliminated.
_recursecall counts drop sharply for the patched version (n=6:137,266 → 4,051 calls) and stay fast at n=8 (394,353 calls, matching
the closed-form combinatorial floor Σ C(nG,k)·P(nH,k) exactly).
hung indefinitely on its final, most complex phase; now completes.