semantic hole?

[Janis Voigtländer]

The mention of "approximate" in http://package.elm-lang.org/packages/Apanatshka/elm-signal-extra/2.3.1/Signal-Time#startTime made me suspicious. And indeed, there is something about timestamps that seems to violate semantic assumptions. (The runtime takes a shortcut that violates the assumed Elm semantics.)

Consider given some signal

s

We make two signals depending on it as follows (for some functions f and g):

s1 = Signal.map f s

s2 = Signal.map g s

We timestamp both signals:

t1 = timestamp s1

t2 = timestamp s2

We should now be safe to assume that (t1 and t2 have their events happening semantically synchronously and) their timestamp portions will always be identical. The documentation of timestamp says so, namely that two signals that rely on the same underlying event source will always have the same timestamp associated with their events. This should apply to s1 and s2 above, right? (This example is not much different from the one in the documentation, where Mouse.x and Mouse.y are used, which both are thought of as images under mapping of Mouse.position.)

It is also not hard to think of scenarios where the correct behavior of an Elm program depends on the assumption that signals built like t1 and t2 above will always have identical timestamps. And yet, the semantic assumption is violated in the actual Elm implementation. The signal built as

test = Signal.map2 (\(a,_) (b,_) -> a == b) t1 t2

will sometimes have an event indicating False instead of True.

I find this worrying.

(To see the issue in action: http://share-elm.com/sprout/54a54f7fe4b05f9c59b24b06)

[Laszlo Pandy]

Yes. I agree with this. There should be one value of time for each event cycle.

Do you think you could send a pull request to fix this?

--
You received this message because you are subscribed to the Google Groups "Elm Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elm-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Janis Voigtländer]

I think fixing this requires a (limited, but non-local) redesign of the signal/event-updating implementation in the runtime. Basically, instead of creating timestamps right when that function is called, in this line: https://github.com/elm-lang/core/blob/master/src/Native/Signal.js#L122, a "current timestamp" should be assigned to each signal. When a signal has an event that triggers events on other signals as well, that timestamp should be taken over for those other signals. And in the Signal.timestamp function, instead of measuring the current time then and there, the timestamp stored in the to-be-stamped signal should be read off. Or maybe the current timestamp should not be assigned to signals themselves, but instead become an explicit argument to the update() functions (in JS).

In any case, various places throughout the runtime would be affected, and I assume that such a deep refactoring is not something Evan would want to accept a PR for. Instead doing it himself. Or at least first having his say on it, possibly also explaining that/why he wants this to stay as it is.

Anyway, I have at least created an issue on this: #75. Evan?

[Janis Voigtländer]

Actually, a fix is likely simpler than I indicated, and requires no deep refactoring. It's not the update() functions that are relevant, but the recv-methods of nodes in the signal graph, and they do already have a timestep argument and propagate it appropriately. So in Native/Signal.js, the timestamp function should not simply rely on a call to liftN, but instead create a new node like the one liftN would create, but where the recv-method is such that it uses its timestep argument as the first component for the output pairs. So, more or less, inlining a specialized version of liftN into these lines: https://github.com/elm-lang/core/blob/7470663e13af7ead52999e8704dfa5ebdf1fd885/src/Native/Signal.js#L121-124 instead of having the "asynchronous" call to localRuntime.timer.now() there.

Still, Evan should have his say first about this issue.

[Evan Czaplicki]

I don't have the mental capacity to load all of this into my brain right now, but it is true that I implemented timestamp in a temporary way. I relied on the fact that JS is pretty slow and things would typically work out, so I could do the feature pretty easily. The intent was always to fix it, and I think I subsequently added the argument to recv but didn't finish? I don't recall.

I'm in favor of making this correction.

[Janis Voigtländer]

Okay, this PR fixes the issue. More specifically, it ensures that there is only one value of time for each event cycle. It does not ensure that initial values of signals will receive the same timestamps. That is, initial values of timestamp s1 and timestamp s2 may have different first components, no matter if s1 and s2 are related somehow. To fix this as well (not only the property on actual events happening), the runtime would have to set a global variable "time-at-program-start" and use that instead of `localRuntime.timer.now()` in that line.

[Jeff Smits]

Maybe you can add a localRuntime.timer.programStart here: https://github.com/elm-lang/core/blob/0aaafb991edb8c2a9dc9225cfc1e43799969bbca/src/Native/Runtime.js#L50

To unsubscribe from this group and stop receiving emails from it, send an email to elm-discuss+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Janis Voigtländer]

Yeah, but adding a global variable to the runtime is something that should really be under Evan's control. :-)

[Janis Voigtländer]

Okay, with Evan's consent, this is now global, in this commit, added to the previous PR, which is now ready to go.