Port temporal reasoning types from the spacetime repository

[ntoxeg]

I have moved the code for the PredictiveImplicationScope type (it has an associated unit test) and information from atom_types.script to the PLN repository, as it seems this should be the place to hold type information and code pertaining to reasoning tasks.

I left Allen Interval Algebra types back in the spacetime repository as I do not know if they are at all used.

This means that projects relying on temporal reasoning do not need to pull spacetime as a dependency now.

[linas]

@ngeiswei I asked @ntoxeg to create this pull req after the conversation in opencog/spacetime#6

There are apparently a handful of atom type declarations in spacetime that have nothing to with spacetime, so putting them into PLN seems to make sense, since I guess PLN uses them?

I just looked at the C++ class in this pull req and its a no-op. It does nothing at all ... It's from @kasim and its from 6 years ago, so it looks like the abandoned start of some project. Since the C++ code does nothing, I recommend just getting rid of it.

[ntoxeg]

Well, the implemented class checks for the number of arguments at least...

But the rest of the code is actually wrong (skipping over the fact that it is not used for anything) - the time lag should be either the first or second argument, not second or third.

[ngeiswei]

The sole existence of this factory is to allow PredictiveImplicationScope to be defined outside of the atomspace repository and yet being treated as a scope link.

_time_interval is indeed set incorrectly, which does not matter as it's not used anywhere.

I have personally no problem having these links in the pln repo. Let me review that PR and merge.

[ngeiswei]

I now remember why PredictiveImplicationScope was moved to spacetime. Some project uses both spacetime and pln, which creates conflicts with

TIME_NODE <- NUMBER_NODE
...

as they are then defined twice.

That's probably why we (@Amenbel and I, if I correctly recall) came to the decision of putting PredictiveImplicationScope in the spacetime repo.

As I said, I have no problem having it in the pln repo, but it looks like the other temporal links will need to remain in the spacetime repo.

[ntoxeg]

As I said, I have no problem having it in the pln repo, but it looks like the other temporal links will need to remain in the spacetime repo.

But doesn’t that mean that, at least in principle, PLN will depend on spacetime? If anything, shouldn’t it be the other way around (and of course this highlights the limitation of how types are being added to Atomese from downstream projects)?

[ngeiswei]

But doesn’t that mean that, at least in principle, PLN will depend on spacetime?

Yes, the temporal reasoning part of PLN then requires spacetime. I'm feeling ambivalent about it, but it looks like that minimizes the pain of other downstream projects for now (maybe @Amenbel can comment on that), so unless someone steps up and clean it up it looks like it's gonna remain that way.

[ntoxeg]

@ngeiswei just an important side note - this means OpenCog PLN will depend on singnet/spacetime as we haven’t reached an agreement to pull the changes upstream.

[ngeiswei]

@ntoxeg, opencog/spacetime should mirror singnet/spacetime. It is due to my own mistake that it doesn't. Either I fix that now, or after PredictiveImplicationLink has been moved to the pln repo.

[ntoxeg]

Well, if temporal types stay in spacetime anyway, then this PR can just be closed, as there is nothing to be done here. Then @ngeiswei can just update opencog/spacetime with singnet/spacetime (as I proposed initially). We can come back to this when the situation with where particular types should be located is sorted out.

[ngeiswei]

PredictiveImplicationScope could stay in spacetime or go to pln. It looks like @linas would prefer if it goes to pln. I'm mostly indifferent. What should we do?

[ntoxeg]

In my view it’s too awkward that PredictiveImplicationScope would be in PLN but PredictiveImplication in spacetime. I would leave this PR be for now, maybe others have some input on this, but as it stands there is nothing to do here anymore.

[ngeiswei]

That's a reasonable view. I'll wait 48h in case someone wants to voice hoh opinion, then merge singnet/spacetime into opencog/spacetime and close that issue.

[linas]

spacetime defines a number of types that it does not need or use; it would be best to move those to the repo that needs & uses them.

Almost all of the types in spacetime/opencog/spacetime/atom-types/atom_types.script are unused by spacetime. A quick grep reveals that spacetime uses only these:

AT_TIME_LINK
TIME_NODE
AT_LOCATION_LINK

As to whether spacetime or PLN should define "basic" links like TIME_NODE, there is an easy answer: if PLN needs access to the octree subsystem that spacetime provides, then obviously, PLN depends on spacetime. If PLN does NOT need the octree subsystem, then it should define something like PLN_TIME_NODE or maybe TEMPORAL_REASONING_NODE, or whatever. The spacetime version could be renamed to SECONDS_SINCE_1970_NODE or something like that. Maybe TIMESTAMP_NODE since it does not actually contain "time" it contains a timestamp of when some event was observed, with some imprecise numerical value, accurate to maybe 10 milliseconds with the current octree code.

The robot code used to use TIME_NODE also, but it did not store seconds, I think it stored nanoseconds.

Moral of the story: different subsystems have a different idea of what "time" is, and those subsystems should be free to define time as they need it. Trying to force all of them to use a single common definition is going to just create problems. Someday in the future, if/when things get more stable and usable, we can revisit this question. For now, its best to keep things as simple as possible.

[linas]

I opened this pull req https://github.com/opencog/spacetime/pull/7 to remove all type definitions that spacetime does not need.

I won't merge it, until a final decision is made here. I think it should be merged, because I think that modular programming is a good thing. Dependencies between different modules should be minimized.

[linas]

@ngeiswei you wrote:

The sole existence of this factory is to allow PredictiveImplicationScope to be defined outside of the atomspace repository and yet being treated as a scope link.

This should "just plain work" without having to write a C++ class for it. That is, even if there's no C++ class, you should be able to just write:

Handle h = atomspace->add(PREDICATIVE_IMPLICATION_SCOPE_LINK, stuff);
ScopeLinkPtr slp = ScopeLinkCast(h);
if (nullptr == slp) printf("the unit test is failing!\n");
else printf("hooray! It worked!\n");

[linas]

OK, one last comment, and then I'll stop.

Some project uses both spacetime and pln

Which project is that? I would like to rename spacetimes TIME_NODE to be TIMESTAMP_NODE to avoid future problems.

The problem is this:

(cog-evaluate!
   (AfterLink
      (TimeNode "Friday afternoon")
      (TimeNode 197684557.421901)))

Does that evaluate to true, or false? How can one ever know? The only plausible solution that I see is to write a custom C++ class for AfterLink that knows how to deal with different time encodings, and how to convert between them. It would also have to deal with rounding errors, because "Friday afternoon plus one nanosecond" is still Friday afternoon, and not really "greater than" in any meaningful sense.

[ngeiswei]

This should "just plain work" without having to write a C++ class for it.

I didn't try but according to @kasimebrahim, it wouldn't. I forgot the technical detail but it had to do with the fact that the link was defined outside of

https://github.com/opencog/atomspace/blob/master/opencog/atoms/atom_types/atom_types.script

[ngeiswei]

I have no problem removing all unused links from spacetime but I suspect other project(s) rely on that. I don't know the name(s) of said project(s). @AmeBel, any idea?

[linas]

it wouldn't.

That's a bug; If it doesn't work, I'll fix the bug.

I suspect other project(s) rely on that

I think they're all unused. All those types date back to ten years ago, the first blocks for embodiment, and the Allen interval code was never written.

[amebel]

In the opencog repo the types defined by spacetime are used in (opencog nlp chatbot), see here and in (opencog nlp relex2logic), see here. These 2 modules are used by (opencog ghost). It would be great if we can keep it functional untill we have something better.

The other uses of the above 2 modules, in the opencog repo, seem to be by unmaintained codes.

[ntoxeg]

Moving types from spacetime to PLN would only require swapping / adding the appropriate dependency and module import in those aforementioned modules.

[linas]

used in (opencog nlp chatbot),

OK, later today, I'll add these types to the opencog repo, and remove them from spacetime. I'll merge those pull reqs soon as they're ready. I'll create a pull req for pln/opencog/pln/types/atom_types.script to add the definition for etc-SCOPE_LINK, but not the C++ class. .. and I'll merge that. I think this will leave everything in a functioning state, everything should work.

I won't add the C++ code for the ScopeLink class, because that seems to be un-needed. If the ScopeLink's aren't working, this is a bug in the atom factory. I won't test for that: so it's up to someone else to test that. If there's a bug in the atom-factory. and someone reports it, I'll fix it.

[linas]

Closing this unmerged; its obsoleted by #41 and #42