test_conditional_instantiation_1 triggers warnings in pattern engine

[linas]

The test_conditional_instantiation_1 unit test of the BackwardsChainerUTest triggers newly-installed errors (now down-graded to warnings) in the pattern engine. .. but unexpectedly passes anyway. The search with the problem has a body that looks like this:

(And
    (And
        (..many EvaluationLinks, some with variables, none are GPN ...))
    (Evaluation
          (GroundedPredicateNode "scm: true-enough")
          (... copy of the second AndLink above ...)))

The problem with this is that the pattern engine gets confused by this search, and does something off-the-wall and wacky... mostly because its trying to be backward-compatible with older styles, and because it didn't do very much checking before starting. Here's what happens:

The second AndLink and the Evaluation-GPN-link are marked as "evaluatable", therefore, no pattern-matching is done. However, the second AndLink contains stuff that are just naked terms, not wrapped in a PresentLink, so it tries to be backwards-compat and grounds one of the terms in it, some term that happens to have a variable in it. As part of the process, though, it moves up to the top - to the second And, and notices "oh hey, this is evaluatable, I better go and evaluate this!" so it goes into that code branch, and (as of a few days ago) notices that there are ungrounded variables, in it (and prints a warning). Well, evaluating that second And is a no-op, because none of the terms inside of it are actually evaluatable, so nothing happens, and the fact that there were ungrounded vars made no difference at all.

Eventually, it gets around to calling the GPN, with the same ungrounded vars in the arguments. and whatever happens .. happens. The unit test passes.

What should be happening is one of two things:

• During pattern compilation, it should check each and every term inside the second AndLink, realize that non of them are actually evaluatable, assume that what you actually wanted was an implicit PresentLink for each of them, and add them to the pool of clauses that need grounding.
• Same as above, but throw an error whenever it finds something that isn't actually evaluatable.

Fixing this in the pattern engine is ... tedious, and based on experience, painful, because there are probably all sorts of weird corner cases that assorted unit tests are testing. Its .. fixable, I guess ... but anyway -- right now, the BackwardChainer is almost surely not doing what you wanted, because the pattern engine is confused and falls into an undocumented/ unsupported code-path.

I opened opencog/atomspace#2631 to track the needed fix in the pattern engine.

Let me know what to do -- should the pattern engine be fixed ASAP, or would it be better to tweak the URE? The work-around is very very simple: just get rid of the second And -- it seems like its really not needed, for any reason that I can tell ... and removing it would be identical to the first fix I propose above!

[linas]

BTW, to add injury to insult -- sometimes, the BCUtest passes, and sometimes it fails. I didn't notice that it sometimes fails, circle-ci was happy, so I merged. But now, its failing more often, and unrelated changes are causing it to get messed up. :-/ ... In particular, I cannot merge opencog/atomspace#2630 even though BCUTest works the same way, before and after that patch.

[linas]

Disabled the unit tests that error out in #88 until this is resolved somehow.

[linas]

This branch fixes things so that BackwardChainer runs, without any errors:

https://github.com/linas/atomspace/tree/mandatory-unquoted-clauses

However, two atomspace unit tests fail. I'm thinking of nuking the failing atomspace unit tests, because I do not believe that are realistic, or do they test wanted behavior. Actually, they are valid tests.

[linas]

Fixed in #2633 ... thank you for paying attention. #88 can now be reverted.

[linas]

Closing. Everything is now fixed. Sorry for the interruption.

[ngeiswei]

OK. I'm glad there's nothing to fix on my part, however I'm seeing that rules of that test do not make use of PresentLink, which I'd like to fix.