rewrite cats.labs.channel for better semantics #136

Merged
merged 1 commit into from Dec 23, 2015

Conversation

Projects
None yet
2 participants
@yanatan16
Contributor

yanatan16 commented Dec 22, 2015

  • bind improvements:
    • handles bind with function returning an empty channel
    • handles bind with function returning channel with multiple values (allowing comprehension)
    • now uses a/pipeline-async to ensure closed correctly
  • simplifies mempty, mzero, mreturn, and mpure using a/to-chan
  • mreturn and mplus check for nil values and instead put :cats.labs.channel/nil on the channel. An empty channel will end computation, and nil values aren't allowed. Generally the (return nil) should only be used when the value is thrown away anyway (such as with guard).
  • mappend/mplus:
    • simplified using a/merge
    • Behavior Change: mappend and mplus now merge channels out of order. I believe this is correct because channels represent streams of data, and ordered merge is not necessarily what is desired all the time. For instance, infinite streams should be able to be mappended, and they could not in the previous code
  • Adds MonadZero implementation, allowing mlet :when form

Passes all tests. Adds tests that fail under current code:

  • channel-with-empty: does a bind where the function returns an empty channel. This causes a "cannot put nil on channel" error currently due to (a/>! c (a/<! result)) statement.
  • channel-comprehension: does a bind where the values multiply. Current code only allows one value per channel returned by bound function, new code allows many values which allows comprehension.
  • semigroup-tests: Adjusted to use sets to compare with mappend due to behavior change
  • monadzero-tests: Test mzero
  • monadplus-tests: Test mplus and mzero laws
  • channel-mlet-when: Test use of :when for in mlet
p/Semigroup
(-mappend [_ sv sv']
- (chain-chans sv sv'))
+ (a/merge [sv sv']))

This comment has been minimized.

@niwinz

niwinz Dec 23, 2015

Member

As far as I understand, merge is not sequential, so it does not a proper impl for mappend. I'm missing something?

@niwinz

niwinz Dec 23, 2015

Member

As far as I understand, merge is not sequential, so it does not a proper impl for mappend. I'm missing something?

This comment has been minimized.

@yanatan16

yanatan16 Dec 23, 2015

Contributor

Yes its a behavior change. I view channels more as streams or sets which are not necessarily sequential. If you were to mappend two infinite channels with chain-chans, you would never see values from the second channel, whereas merge can handle infinite channels, but doesn't give any sequential guarantees.

@yanatan16

yanatan16 Dec 23, 2015

Contributor

Yes its a behavior change. I view channels more as streams or sets which are not necessarily sequential. If you were to mappend two infinite channels with chain-chans, you would never see values from the second channel, whereas merge can handle infinite channels, but doesn't give any sequential guarantees.

This comment has been minimized.

@niwinz

niwinz Dec 23, 2015

Member

I know that and I understand the advantages of using merge. My unique concern is about correctness vs practical approach. How is correct you approach in terms of mappend laws?

@niwinz

niwinz Dec 23, 2015

Member

I know that and I understand the advantages of using merge. My unique concern is about correctness vs practical approach. How is correct you approach in terms of mappend laws?

This comment has been minimized.

@yanatan16

yanatan16 Dec 23, 2015

Contributor

Like many sets, there are probably many options for which operation to choose from to create a Monoid.

The three laws are left identity, right identity, and associativity (https://en.wikipedia.org/wiki/Monoid#Definition).
Associativity is: a <> (b <> c) = (a <> b) <> c (where <> represents an infix mappend)

merge satisfies the identity laws when mempty is an empty, closed channel. It also satisfies the associativity law under this form of equality (defn chan-eq [c d] (= (<!! (a/into #{} c)) (<!! (a/into #{} d)))).

If you think of channels as being equal only if they are ordered the same, then no, merge does not satisfy the associativity law. But channels are the binding of time and data, and since time is inherently unpredictable in computing, I think it is wrong to include ordering (or time) when defining channel equality. So I would define channel equality to be the set-based method above, and therefore merge would satisfy the laws.

I looked around for some other instances of streams/channels having monoid instances but didn't find many:

  • scalaz.StreamT seems to agree with the chain-chans method (ala ordered lists)
  • Haskell's pipes performs an mappend on the inner values (ala maybe)

I think all three approaches can be correct under some equality operator. But perhaps the best of both worlds is the inner mappend like the pipes library uses. That would allow ordering to be preserved and infinite streams to be handled

A rough sketch:

(defn mappend-chans [c d]
  (go-loop []
    (let [vc (<! c) vd (<! d)]
      (when (and vc vd)
        (>! out (mappend vc vd))
        (recur)))))

(<!! (a/into [] (m/append (a/to-chan [[1]]) (a/to-chan [[2] [3]])))) ; => [[1 2]]
@yanatan16

yanatan16 Dec 23, 2015

Contributor

Like many sets, there are probably many options for which operation to choose from to create a Monoid.

The three laws are left identity, right identity, and associativity (https://en.wikipedia.org/wiki/Monoid#Definition).
Associativity is: a <> (b <> c) = (a <> b) <> c (where <> represents an infix mappend)

merge satisfies the identity laws when mempty is an empty, closed channel. It also satisfies the associativity law under this form of equality (defn chan-eq [c d] (= (<!! (a/into #{} c)) (<!! (a/into #{} d)))).

If you think of channels as being equal only if they are ordered the same, then no, merge does not satisfy the associativity law. But channels are the binding of time and data, and since time is inherently unpredictable in computing, I think it is wrong to include ordering (or time) when defining channel equality. So I would define channel equality to be the set-based method above, and therefore merge would satisfy the laws.

I looked around for some other instances of streams/channels having monoid instances but didn't find many:

  • scalaz.StreamT seems to agree with the chain-chans method (ala ordered lists)
  • Haskell's pipes performs an mappend on the inner values (ala maybe)

I think all three approaches can be correct under some equality operator. But perhaps the best of both worlds is the inner mappend like the pipes library uses. That would allow ordering to be preserved and infinite streams to be handled

A rough sketch:

(defn mappend-chans [c d]
  (go-loop []
    (let [vc (<! c) vd (<! d)]
      (when (and vc vd)
        (>! out (mappend vc vd))
        (recur)))))

(<!! (a/into [] (m/append (a/to-chan [[1]]) (a/to-chan [[2] [3]])))) ; => [[1 2]]

This comment has been minimized.

@niwinz

niwinz Dec 23, 2015

Member

Hmm, thank you very much for the clear explanation. I think that your first approach is the desired so no change is needed.

@niwinz

niwinz Dec 23, 2015

Member

Hmm, thank you very much for the clear explanation. I think that your first approach is the desired so no change is needed.

@niwinz

This comment has been minimized.

Show comment
Hide comment
@niwinz

niwinz Dec 23, 2015

Member

In general very nice work! Thanks for your time!

Member

niwinz commented Dec 23, 2015

In general very nice work! Thanks for your time!

niwinz added a commit that referenced this pull request Dec 23, 2015

Merge pull request #136 from yanatan16/patch-channels
rewrite cats.labs.channel for better semantics

@niwinz niwinz merged commit 0a84607 into funcool:master Dec 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yanatan16 yanatan16 deleted the yanatan16:patch-channels branch Dec 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment