glob interval not supporting disjoint unions about atomspace

[sikefield3]

@linas I'm going to write the error message, ok?

[linas]

The code is misleading. The intent is that either the interval is specified, or it is not; if there are two (or more) intervals given, that is an error, and should be flagged as an error. In other words, extend() should never be called more than once.

Here's the misleading part:

• make_interval() creates the invalid interval (MAX, 0)
• extend() applied to the above and (L,U) always gives (L,U) as the result.
• At the end, there are tests to look for the invalid interval, and to replace it by the default. (Because if it's invalid at the end, that means the interval was never set).

I suppose the above could have been written differently, in a less misleading fashion. Maybe with a bool flag not_been_set_yet instead of an invalid value. But that amounts to "the same thing".

The extend() function could be (should be??) re-written as:

extend(lhs) {
 if (rhs==invalid) return lhs;
if (lhs==invalid) return rhs;
throw ("Must not specify interval twice!");
}

Huh. Umm, except that foo==invalid is what TypeChoice::is_empty() does. So extend should be:

static inline GlobInterval extend(const GlobInterval& lhs,
                                  const GlobInterval& rhs)
{
   if (TypeChoice::is_empty(lhs)) return rhs;
   if (TypeChoice::is_empty(rhs)) return lhs;
   throw (Must not specify interval twice!); 
}

The current extend() implementation is left over from some earlier half-baked ideas. I think. The unit tests know for sure.

[sikefield3]

static inline GlobInterval extend(const GlobInterval& lhs,
                                  const GlobInterval& rhs)
{
   if (TypeChoice::is_empty(lhs)) return rhs;
   if (TypeChoice::is_empty(rhs)) return lhs;
   throw (Must not specify interval twice!); 
}

The current extend() implementation is left over from some earlier half-baked ideas. I think. The unit tests know for sure.

The tests work with this change.

[sikefield3]

The code is misleading. The intent is that either the interval is specified, or it is not; if there are two (or more) intervals given, that is an error, and should be flagged as an error. In other words, extend() should never be called more than once.

Here's the misleading part:

* `make_interval()` creates the invalid interval `(MAX, 0)`

So extend() is the method to create the interval one is interested in ?

GlobInterval TypeChoice::make_interval(const HandleSeq& ivl)
{

	long lb = std::lround(NumberNodeCast(ivl[0])->get_value());
	long ub = std::lround(NumberNodeCast(ivl[1])->get_value());

But couldn't make_interval() create a valid interval also, so that we don't need extend() ?

[linas]

Can I give a meta-level answer?

The primary goal is to have the Atomese expressions work well, and not be surprising or weird. Whatever C++ code accomplishes that is OK (I would find it acceptable), and whatever C++ code is the simplest and is the least misleading is best.

With that out of the way ... so ... how is the Atomese supposed to behave? Well, I look at the wiki, https://wiki.opencog.org/w/GlobNode and all of the examples have exactly one IntervalLink in them, and so they are never intersected or unioned or overlapped, so any extra logic for unions or intersections is not needed ... at least, for that use.

Next I click over to https://wiki.opencog.org/w/IntervalLink and it says even less. Except for one curious thing: it mentions "TimeInterval" I click thought and ... hmm disappointment.

To conclude: in this particular blob of code, where intervals are being used to indicate a range for glob matching, the current code is good enough. It's misleading ... but its not wrong. It could be made less misleading.

Now, there has been a different, distantly-related idea about "time intervals", and there has been much discussion about that over the years. If you are curious, I can repeat it; however, my basic take is that time intervals don't belong in the fundamental base AtomSpace. If someone wants to do them in some other repo, e.g. in https://github.com/opencog/spacetime, that's fine. I'd rather keep the base AtomSpace as small as reasonable.

[sikefield3]

The primary goal is to have the Atomese expressions work well, and not be surprising or weird. Whatever C++ code accomplishes that is OK (I would find it acceptable), and whatever C++ code is the simplest and is the least misleading is best.

OK. I think I understand now.
Best I just close this off ...

[linas]

OK, thanks for the work! It's good work! I appreciate it! More!