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

Change location of where ListLink wrapping occurs. #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

linas
Copy link
Contributor

@linas linas commented Mar 17, 2020

As discussed in issue #164.

Untested -- I have not tried running this code; I think it's correct
but I might have made a mistake, or missed a spot where a change was
needed.

As discussed in issue MOZI-AI#164.

Untested -- I have not tried running this code; I think it's correct
but I might have made a mistake, or missed a spot where a change was
needed.
@tanksha
Copy link
Contributor

tanksha commented Mar 18, 2020

@linas I think its better to get rid of them at all. As the GroundedSchemaNodes are removed, there is no need for extra ListLink to wrap the output. I will be working on this

@linas
Copy link
Contributor Author

linas commented Mar 18, 2020

I don't understand at all. The ListLink here has nothing at all to do with GroundedSchemaNodes.... they are completely unrelated. The GroundedSchema never needed these... The ListLink here is grouping together a bunch of clauses, for some other unknown reason.

@tanksha
Copy link
Contributor

tanksha commented Mar 26, 2020

@linas yes, the GroundedSchemaNodes have nothing to do with this but moving the ListLink up will create a much bigger number of outgoing nodes for the ListLink which the Pattern miner code is complains.

@linas
Copy link
Contributor Author

linas commented Mar 26, 2020

moving the ListLink up will create a much bigger number of outgoing nodes

This does not change the number of nodes/links created, at all: exactly the same structures are created, either way. All that this does is refactor the code-base. It does not change the output of the codebase.

@tanksha
Copy link
Contributor

tanksha commented Mar 26, 2020

@Habush I will merge this PR as it fixes the issue with having an empty ListLink you were getting from Biogrid. can you check?

@linas
Copy link
Contributor Author

linas commented Mar 26, 2020

empty ListLink

This pull req does not change the output: it still produces empty list links in all the same places as before.

Copy link
Contributor

@tanksha tanksha left a comment

Choose a reason for hiding this comment

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

I thought you have replaced the ListLinks with scheme list here?

annotation/functions.scm Show resolved Hide resolved
@linas
Copy link
Contributor Author

linas commented Mar 26, 2020

Yes, exactly, it replaces 9 different locations by only two. That should make it easier to remove those two, if/when you are ready to do that, rather than struggling with all 9.

@tanksha
Copy link
Contributor

tanksha commented Mar 26, 2020

The ListLink here is grouping together a bunch of clauses, for some other unknown reason.

To be clear on why we were using ListLink

at first, the generate-result function was called through a GroundedSchemaNode, and when there is no result to be returned, we were having #unspecified variable error because the pattern matching query expects an opencog atom as a result, thats why we send an empty ListLink.

and when there is a result, since we were sending a bunch of clauses (more than one EvaluationLinks) as a result, we need to wrap all in one ListLink and return.

But now, we don't have a GroundedSchemaNode ... but when you refactor the code, since you dont want to modify the output, you keep the ListLink but we dont need them anymore as it was for the above reason we used them HERE in the first place.

@linas
Copy link
Contributor Author

linas commented Mar 26, 2020

#unspecified

It is impossible for the pattern matcher to ever see *unspecified*, since this is a scheme-only object. Atomese also never-ever generates *unspecified*. It is only generated by guile - for example, (if #f "foo") evaluates to *unspecified* because one of the two if-statement branches is unspecified. There are lots of if's in the code-base, and the *unspecified* is coming from one of them.

since we were sending a bunch of clauses (more than one EvaluationLinks) as a result, we need to wrap all in one ListLink and return.

Why? That is the original question: why do you need to wrap them? Oh, I understand now.

GroundedSchemaNode

The use of the ListLinks has nothing at all to do with the GroundedSchema. They are completely unrelated to one-another. The code generates the same results before, and after. One could have eliminated the ListLinks entirely, and still used GSN's for everything; it would have made no difference. Oh I understand now.

@linas
Copy link
Contributor Author

linas commented Mar 26, 2020

Here's a better example:

(define ttt #t)
(define u (if (not ttt) "foo"))
(equal? u *unspecified*)
(display u)(newline)

@linas
Copy link
Contributor Author

linas commented Mar 26, 2020

Oh... I understand now ...

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.

2 participants