Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use AndLink instead of SequentialAndLink for the psi-rules #2976

Merged
merged 1 commit into from
Jan 17, 2018

Conversation

leungmanhin
Copy link
Contributor

This should fix both OpenPsiImplicatorUTest and OpenPsiSCMUTest.

@leungmanhin leungmanhin merged commit 172b087 into opencog:master Jan 17, 2018
@leungmanhin leungmanhin deleted the psi branch January 17, 2018 10:24
@linas
Copy link
Member

linas commented Jan 17, 2018

Hmm. Well, sequentialAnd is ordered, while And is not ordered, so potentially you cannot tell the first from the second.

Is an AndlInk actually needed here? Maybe just a plain-old OrderedLink would be enough?

@linas
Copy link
Member

linas commented Jan 17, 2018

I mean, it was throwing an error because there;'s a ConceptNode in the SequntialAnd .. however, a ConceptLink in a regular And is also very illegal...

@ngeiswei
Copy link
Member

ConceptNode in a regular And should not be illegal. If it is then we need do define an And connector for Concept, as opposed to overloading AndLink for both concepts and predicates.

@ngeiswei
Copy link
Member

Regarding using SequentialAnd vs And vs something else. Since currently no PLN reasoning is taking place it's not currently important. In principle I guess both SeqAnd and And could be used, they do mean different things but both can be of value to a cognitive decision process. I think it is however a mistake to use the order to distinguish the context from the action. Rather the action should be identified by its form, maybe one could use

Predicate "do"
  <action>

or something of that sort. It is interesting to note that the hypothetical actualization of the action is in someway part of a greater context, the whole antecedent of the rule. But again, since PLN isn't used yet, using the order is still OK for now.

@leungmanhin
Copy link
Contributor Author

I think we no longer need to use SeqAnd for "context" and "action". It was there because the order was used to distinguish the context from action. But as Nil said that's not good, also getting context/action/goal of a psi-rule is handled differently now: https://github.com/opencog/opencog/blob/master/opencog/openpsi/OpenPsiRules.cc#L115-L142
and it doesn't need the "order" to distinguish one from another. So I think it's ok to use AndLink.

@linas
Copy link
Member

linas commented Jan 18, 2018

OK, I have two issues:

  1. what does it mean to take the "and" of things that are not predicates? are you using "and" to mean "set-union"? If so, we should create a SetUnionLink -- this would be handy, because it would allow probabilistic pattern matching -- e.g. the pattern matcher was set up to handle SetUnionLink, etc. but that wasn't implemented because it wasn't urgent back then.

  2. SequentialAnd still makes a lot of sense for psi rules, even if it is not being used currently. For a while, I was trying to write robot rules, that were "if someone entered the room, and a little while later a second person entered the room.." and the SequentialAnd allowed this sequencing to happen. There are also other cases, where you want to have narrow psi-rules (applying to narrow situations) to be tried first, and then broader ones, only if the narrow match fails. So I think in the long-run, sequential-and still makes a lot of sense.

@linas
Copy link
Member

linas commented Jan 18, 2018

The current code-base uses AndLink for all sorts of things. I'm arguing that it could be, should be tightened up. Or something. Its OK to sometimes leave things loose and poorly defined; but sometimes its better not to...

@leungmanhin
Copy link
Contributor Author

leungmanhin commented Jan 18, 2018

This reminds me when doing pattern matching, some of the stuffs may be "patterns" that should be looked for, and some may be "evaluatable" or "executable", and they are all under an AndLink...

BTW, just to be clear, this AndLink I changed is a high level one:

(Implication
  (And ; <-- This one
    context
    action
  )
  goal
)

So in the example you gave, it will be written:

(Implication
  (And
    (SequentialAnd
      ; someone entered the room
      ; a little while later a second person entered the room
    )
    action
  )
  goal
)

And I agree it makes sense to use SequentialAnd for that.

@linas
Copy link
Member

linas commented Jan 18, 2018

Yeah, the pattern matcher uses AndLink. The long-term plan is to use PresentLink and AbsentLink to indicate what should be looked for. For now, having only implicit PresentLink seems OK, and does not lead to confusion.

Recall that, in the begining, long ago, BindLink was called ImplicationLink. But then we got confused about variable binding and changed the name to BindLink which wrapped around an Implication. And then Nil removed the Implication because it did not seem to be needed. And then Ben started talking about openpsi as a different kind of implication. And then the idea of variable scoping became clear, so ScopeLink got invented when that became clear. And then Nil Invented ImplicationScopeLink, so we kind-of went in a full circle back to the beginning, where we started. Its OK, its confusing.

@ngeiswei
Copy link
Member

I agree about AndLink being renamed to say IntersectLink (not union) rather than being overloaded.

@ngeiswei
Copy link
Member

An additional comment about openpsi rule format is that they should very likely be PredictiveImplicationLink rather than Implication, but that's not important for now...

@amebel
Copy link
Contributor

amebel commented Jan 23, 2018

As a reminder there was the following discussion with regards to openpsi representation, #2899 (comment)

@amebel amebel added the openpsi label Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants