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 cond link #2066

Merged
merged 13 commits into from
Feb 12, 2019
Merged

Add cond link #2066

merged 13 commits into from
Feb 12, 2019

Conversation

kasimebrahim
Copy link
Contributor

return HandleCast(inst.instantiate(default_exp, HandleMap()));
}

DEFINE_LINK_FACTORY(CondLink, COND_LINK)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DEFINE_LINK_FACTORY(CondLink, COND_LINK)
DEFINE_LINK_FACTORY(CondLink, COND_LINK)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Nil says. Your editor seems to forget to inert a newline.

tests/atoms/condlink.scm Outdated Show resolved Hide resolved
Copy link
Member

@linas linas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice! I don't really have any complaints; I don't have any major complaints; all my remarks are about style, rather than design. Fix them up, and I have no problems with a merge.

opencog/atoms/atom_types/atom_types.script Outdated Show resolved Hide resolved
opencog/atoms/atom_types/atom_types.script Outdated Show resolved Hide resolved
opencog/atoms/execution/CMakeLists.txt Outdated Show resolved Hide resolved
// using true_link as condition should do it.
if (i != 0 && LIST_LINK == _outgoing[i - 1]->get_type())
throw SyntaxException(TRACE_INFO,
"CondLink is expected to wrap expressions in LIST_LINK.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this check, and the checks below, it would be better to check if the even atoms are evaluatable, and the odd ones are executable. nameserver().isA(type, EVALUATABLE_LINK) and nameserver.isA(type, EXECUTABLE_LINK) The EVALUATABLE_LINK type already exists, and "does the right thing"; the EXECUTABLE_LINK does not yet exist) (I started adding it a few weeks ago, but did not finish. Perhaps you can add it, in a separate pull req?)

for (unsigned i = 0; i < conds.size(); ++i) {
tvp = EvaluationLink::do_evaluate(scratch, conds[i]);
if (tvp->get_mean() > 0.5)
return HandleCast(inst.instantiate(exps[i], HandleMap()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not wrong, but there is now a new and better way of doing this: call exps[i]->execute(scratch).

This generic execute() method is brand new, introduced just a week ago, and it is meant to resolve multiple issues with calling the instantiator directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @linas, In case of eager execution the expressions in the CondLink might have been already executed right? such as an ArithmeticLink might have been already reduced to a NumberNode and calling execute on that NumberNode should throw the "Not Executable" exception in Atom.h? or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for eager evaluation, the arguments will already have been reduced. There should not be any exceptions as a result, as the reduction is (meant to be) performed only once, not twice.

Lay evaluation would be better; it almost works; there's some open issue saying "must finish this work. Anyway, toe CondLink code looks OK to me.

return HandleCast(inst.instantiate(default_exp, HandleMap()));
}

DEFINE_LINK_FACTORY(CondLink, COND_LINK)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What Nil says. Your editor seems to forget to inert a newline.

opencog/atoms/execution/Instantiator.cc Show resolved Hide resolved
tests/atoms/CondLinkUTest.cxxtest Outdated Show resolved Hide resolved
@@ -0,0 +1,59 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding one test for EvaluationLink with grounded predicate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @noskill my scheme knowledge is very limited, will this suffice

(define (grounded_cond1 ) (cog-new-stv 0 0))

(define (grounded_cond2 ) (cog-new-stv 1 1))

(define grounded-cond
    (CondLink
	(Evaluation
	     (GroundedPredicate "scm:grounded_cond1")
	     (List ))
	(NumberNode 1)

	(Evaluation
	     (GroundedPredicate "scm:grounded_cond2")
	     (List ))
	(NumberNode 2)))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like it was added

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks good, thanks

@linas linas merged commit 4284d8c into opencog:master Feb 12, 2019
@linas
Copy link
Member

linas commented Feb 12, 2019

Thanks! This looks good. Sorry for all the earlier confusion about whether this was needed or not.

; Expression for the above condition.
(PlusLink (Number 3)(Number 2))
; This is the second condition
(FalseLink )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the same as?:

else if false: 
     (PlusLink (Number 1)(Number 2))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

(FalseLink )
(PlusLink (Number 1)(Number 2)))
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an example of common usecase:

if (expression) 
     some code
else
      some other code

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean like this?

(CondLink
  |expression|                  ; EvaluatableLink
  |some-code|                  ; if |expression| evaluates to true
  |some-other-code|          ; else
)

https://wiki.opencog.org/w/CondLink

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@kasimebrahim kasimebrahim deleted the add-CondLink branch February 21, 2019 11:18
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.

5 participants