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

Add SetLink to the list #2814

Closed
wants to merge 3 commits into from
Closed

Add SetLink to the list #2814

wants to merge 3 commits into from

Conversation

tanksha
Copy link

@tanksha tanksha commented Apr 30, 2021

This will make creating patterns like (Not (Set A)) possible, in working with singletons.

@tanksha tanksha requested a review from ngeiswei April 30, 2021 17:45
@ngeiswei
Copy link
Member

ngeiswei commented May 5, 2021

@tanksha, I think it would be more appropriate to be grouped with that code

// Allow conjunction, disjunction and negation of concepts as
// well, in that case these are interpreted as intersection,
// union and complement. Since it cannot inherit from
// EVALUATABLE_LINK (cause it's a Node) we have to add it here.
if (h->is_type(CONCEPT_NODE)) continue;

since SetLink can be interpreted as a concept. Would be good to have a comment explaining that as well.

@linas
Copy link
Member

linas commented May 5, 2021

Nil,

The meta-issue is that none of these link types are actually "evaluatable" in Atomese. Here's the problem: in the early days of "Atomese", at Hanson Robotics, there was a need to (cog-evaluate! (SequentialAnd (recognize-person) (Or (smile) (blink) (say-hello)))) which means that recognize-person had to be an atom that returned a (crisp) truth-value - if it returned false, the evaluation would stop. If one of the atoms was not actually evaluatable (e.g. was a ConceptNode) it would crash. That is because there is no C++ ConceptNode::evaluate() method. So the checker was invented to catch these kinds of errors early, while the system was being booted, instead of late, when the robot was onstage.

So now you see what the problem is -- if you call (cog-evaluate! (And (Concept "foo"))) or (cog-evaluate! (Not (Set))) an exception will be thrown.

There are 3-4 different solutions:

  1. get rid of check_evaluatable and instead create a robot-code-static-typechecker module, and another called pln-static-typechecker

  2. invent something that would allow pln to overload the C++ ConceptNode::evaluatie() method with custom code that does what PLN needs it to do. (I've got several ideas on how to do this)

  3. remove large parts of atomese from the atomspace, and move it to "robot-atomese" and "pln atomese" so that what's left in the atomspace is "common atomese that everyone uses".

I'll open a new issue to continue this discussion, we shouldn't have it here... See #2816

@ngeiswei
Copy link
Member

ngeiswei commented May 5, 2021

I, BTW, would have no problem introducing UnionLink, IntersectLink and ComplementLink instead of AndLink, OrLink and NotLink for concepts. That would remove some confusion as well. But it would probably require a cascade of changes in the user land.

@linas
Copy link
Member

linas commented May 5, 2021

So -- UnionLink, IntersectLink and ComplementLink -- one advantage would be to compute numerical values in C++ code, where its fast, instead of calling (GroundedShema "scm:compute-union-tv") which is slow, due to the need to bounce between C++ and scheme.... let me pick this up in #2816

@linas
Copy link
Member

linas commented May 5, 2021

Hmm. As I attempt to think about #2816, I realize that perhaps UnionLink etc is the simplest solution that makes all my vague and hard-to-describe confusions melt away and/or be deferred to some distant future. Let me write a more detailed proposal there. This could all be pretty simple.

@tanksha
Copy link
Author

tanksha commented May 17, 2021

@ngeiswei I have made the suggested changes, please check.

@linas
Copy link
Member

linas commented Sep 7, 2021

Where are we at with this? It seems that whatever needed this, it was not urgent. May I close this?

@ngeiswei
Copy link
Member

ngeiswei commented Sep 9, 2021

I think @tanksha 's change is correct for the time being. The better longer term change could be to introduce UnionLink, etc. However I have an important, and possibly relevant to that issue, change to suggest before that, that I will write an issue about soon. But in the meantime, before these more important changes, I think we can accept that change.

However it's failing circleci. @tanksha, could you rebase your branch (oh, it's your master branch, you should use another one) onto the opencog/master?

@ngeiswei
Copy link
Member

ngeiswei commented Sep 9, 2021

@tanksha, I think you need to close that PR and create a new one (adding a reference to that one cause it discusses important matters) with a new branch.

@ngeiswei
Copy link
Member

ngeiswei commented Sep 9, 2021

That other option is to just close that PR and forget about it, but I believe the bio-atomspace relies on that change.

linas added a commit to linas/atomspace that referenced this pull request Sep 9, 2021
@linas
Copy link
Member

linas commented Sep 9, 2021

Pull req #2843 adds UnionLink. I plan to merge it as soon as it passes unit tests.

I still think that experimenting with novel kinds of truth values is worthwhile: e.g. having a IndirectEvidenceTruthValue that knew how to "calculate itself" on demand. (see #2816 for a very short sketch) @ngeiswei I believe you currently use GroundedPredicateNode "scm:foo" for this. I'm suggesting that doing it in C++ would be faster, and doing it in a TV rather than a Node/Link would simplify formulas and simplify rules. Maybe - that's why I call it an "experiment". (Simplifying anything tends to open new horizons)

@linas
Copy link
Member

linas commented Sep 9, 2021

failing circleci.

Just hit "retest" I don't think you have to rebase.

@ngeiswei
Copy link
Member

I believe we can safely close that issue now.

@ngeiswei ngeiswei closed this Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants