From 6674aac3c248b33f8e1b8b869a193666798ebcbe Mon Sep 17 00:00:00 2001 From: Linas Vepstas Date: Thu, 4 Nov 2021 21:40:22 -0500 Subject: [PATCH 1/2] Provide a superior test for teh need for dummies --- opencog/atoms/pattern/PatternLink.cc | 36 +++++++++++++++++++++------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/opencog/atoms/pattern/PatternLink.cc b/opencog/atoms/pattern/PatternLink.cc index 718017c5e6..dd648c39c5 100644 --- a/opencog/atoms/pattern/PatternLink.cc +++ b/opencog/atoms/pattern/PatternLink.cc @@ -1128,21 +1128,39 @@ void PatternLink::make_term_tree_recursive(const PatternTermPtr& root, if ((parent->getHandle() == nullptr or parent->hasEvaluatable()) and not ptm->isQuoted() and can_evaluate(h)) { - - // Yuck. Ugly, ugly hack to pass the FowardChainerUTest. - // Need to remove this ASAP. - bool already_have_fixed = false; - for (const PatternTermPtr& man : _pat.pmandatory) - if (not man->hasAnyEvaluatable()) + // Are there any variables below us? + // Do they already appear in some existing mandatory term? + // If not, then we have to go fishing for fixed terms, + // or add dummies. + // FIXME: It might be conceptually cleaner to do this in a + // second pass, after the `make_term_tree()` has been done + // on all the clauses. But, for now, we do this here. + bool need_dummies = false; + HandleSet vset = get_free_variables(h); + for (const Handle& v: vset) + { + bool found_this_v = false; + for (const PatternTermPtr& man : _pat.pmandatory) { - already_have_fixed = true; + HandleSet vman = get_free_variables(man->getHandle()); + if (vman.end() != vman.find(v)) + { + found_this_v = true; + break; + } + } + if (not found_this_v) + { + need_dummies = true; break; } + } + if (not need_dummies) return; // If its an AndLink, make sure that all of the children are // evaluatable. The problem is .. users insert AndLinks into // random places... - if (not already_have_fixed and AND_LINK == t) + if (AND_LINK == t) { for (const PatternTermPtr& ptc : ptm->getOutgoingSet()) if (ptc->isQuoted() or not can_eval_or_present(ptc->getHandle())) @@ -1153,7 +1171,7 @@ void PatternLink::make_term_tree_recursive(const PatternTermPtr& root, } // If the evaluatables have literal-ish subterms, add those. - if (not already_have_fixed and add_unaries(ptm)) return; + if (add_unaries(ptm)) return; // If the above failed, then try adding dummy variables. add_dummies(ptm); From 22dceff486dc25536443bb8455db83999b542845 Mon Sep 17 00:00:00 2001 From: Linas Vepstas Date: Thu, 4 Nov 2021 21:57:18 -0500 Subject: [PATCH 2/2] Move brand-new blob of code to it's own method. --- opencog/atoms/pattern/PatternLink.cc | 58 ++++++++++++++-------------- opencog/atoms/pattern/PatternLink.h | 3 +- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/opencog/atoms/pattern/PatternLink.cc b/opencog/atoms/pattern/PatternLink.cc index dd648c39c5..5100b30a5b 100644 --- a/opencog/atoms/pattern/PatternLink.cc +++ b/opencog/atoms/pattern/PatternLink.cc @@ -768,6 +768,35 @@ bool PatternLink::is_virtual(const Handle& clause) /* ================================================================= */ +/// Return true if the term has variables in it that do not yet appear +/// in any non-evaluatable mandatory clause. If there are such +/// variables, we either have to find non-evaluatable terms that hold +/// them, or add those variables directly, as mandatory terms. +bool PatternLink::need_dummies(const PatternTermPtr& ptm) +{ + // Are there any variables below us? + // Do they already appear in some existing mandatory term? + // If not, then we have to go fishing for fixed terms, + // or add dummies. + HandleSet vset = get_free_variables(ptm->getHandle()); + for (const Handle& v: vset) + { + bool found_this_v = false; + for (const PatternTermPtr& man : _pat.pmandatory) + { + HandleSet vman = get_free_variables(man->getHandle()); + if (vman.end() != vman.find(v)) + { + found_this_v = true; + break; + } + } + if (not found_this_v) + return true; + } + return false; +} + /// Add implicit fixed terms. The pattern might consist only of /// evaluatable clauses. Unless we find some constant Atom inside of /// it, we risk having to perform a very large search trying to find @@ -1128,34 +1157,7 @@ void PatternLink::make_term_tree_recursive(const PatternTermPtr& root, if ((parent->getHandle() == nullptr or parent->hasEvaluatable()) and not ptm->isQuoted() and can_evaluate(h)) { - // Are there any variables below us? - // Do they already appear in some existing mandatory term? - // If not, then we have to go fishing for fixed terms, - // or add dummies. - // FIXME: It might be conceptually cleaner to do this in a - // second pass, after the `make_term_tree()` has been done - // on all the clauses. But, for now, we do this here. - bool need_dummies = false; - HandleSet vset = get_free_variables(h); - for (const Handle& v: vset) - { - bool found_this_v = false; - for (const PatternTermPtr& man : _pat.pmandatory) - { - HandleSet vman = get_free_variables(man->getHandle()); - if (vman.end() != vman.find(v)) - { - found_this_v = true; - break; - } - } - if (not found_this_v) - { - need_dummies = true; - break; - } - } - if (not need_dummies) return; + if (not need_dummies(ptm)) return; // If its an AndLink, make sure that all of the children are // evaluatable. The problem is .. users insert AndLinks into diff --git a/opencog/atoms/pattern/PatternLink.h b/opencog/atoms/pattern/PatternLink.h index 50b0892a5e..3e498891fb 100644 --- a/opencog/atoms/pattern/PatternLink.h +++ b/opencog/atoms/pattern/PatternLink.h @@ -133,8 +133,9 @@ class PatternLink : public PrenexLink void locate_cacheable(const PatternTermSeq& clauses); - void add_dummies(const PatternTermPtr&); + bool need_dummies(const PatternTermPtr&); bool add_unaries(const PatternTermPtr&); + void add_dummies(const PatternTermPtr&); void make_connectivity_map(void); void make_map_recursive(const Handle&, const PatternTermPtr&);