Added `merge` (product) to `Arrow` for arrows composition #2063

marcobattaglia · 2017-12-05T16:30:25Z

merge &&&

scala> import cats.implicits._
scala> import cats.arrow.Arrow
scala> val addEmpty: Int => Int = _ + 0
scala> val multiplyEmpty: Int => Double= _ * 1d
scala> val f: Int => (Int, Double) = addEmpty &&& multiplyEmpty
scala> f(1)
res0: (Int Double) = (1,1.0)

combineAndByPass -<

Create a new computation F that apply f andThen biFork the result
one way is applied to g and the other pass through
the result is a tuple

                 /-----out1 = f(input) -----\
      input ----{                            }----(out1,out2)
                 \-----out2 = g(f(input))---/

Example:

     scala> import cats.implicits._
     scala> import cats.arrow.Arrow
     scala> val twoTimes: Int => Double = _ * 2d
     scala> val fiveTimes: Double => Double= _ * 5
     scala> val f: Int => (Double, Double) = twoTimes -< fiveTimes
     scala> f(2)
     res0: (Double Double) = (4.0,20.0)

fix tests

…cats.arrow.Arrow

kailuowang · 2017-12-05T16:57:11Z

will need to add some mima exceptions

exclude[ReversedMissingMethodProblem]("cats.arrow.Arrow.merge"),
exclude[ReversedMissingMethodProblem]("cats.arrow.Arrow.combineAndByPass"),
exclude[ReversedMissingMethodProblem]("cats.arrow.Arrow#Ops.merge"),
exclude[ReversedMissingMethodProblem]("cats.arrow.Arrow#Ops.combineAndByPass"),
exclude[ReversedMissingMethodProblem]("cats.arrow.Arrow#Ops.&&&"),
exclude[ReversedMissingMethodProblem]("cats.arrow.Arrow#Ops.-<"),

to here

kailuowang · 2017-12-05T17:04:39Z

I took the liberty to update your comment with some formatting. Hope you don't mind.

…ombineAndByPass and relative syntax Ops

marcobattaglia · 2017-12-05T17:16:05Z

HI Kailuowang. I have committed the exclude directives. Do I need to do another pull request or is it ok for you?
Thanks for your better formatting.

kailuowang · 2017-12-05T17:19:25Z

You are welcome! Everything related in one PR would be better.

kailuowang · 2017-12-05T21:00:34Z

some errors in the doctest

m/home/travis/build/typelevel/cats/core/.jvm/target/scala-2.11/src_managed/test/cats/arrow/ArrowDoctest.scala:46: identifier expected but ')' found.
/home/travis/build/typelevel/cats/core/.jvm/target/scala-2.11/src_managed/test/cats/arrow/ArrowDoctest.scala:50: ')' expected but '}' found.
/home/travis/build/typelevel/cats/core/.jvm/target/scala-2.11/src_managed/test/cats/arrow/ArrowDoctest.scala:65: identifier expected but ')' found.�
/home/travis/build/typelevel/cats/core/.jvm/target/scala-2.11/src_managed/test/cats/arrow/ArrowDoctest.scala:69: ')' expected but '}' found.�

To run doctest, sbt coreJVM/test

codecov-io · 2017-12-05T23:56:39Z

Codecov Report

Merging #2063 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2063      +/-   ##
==========================================
+ Coverage   94.66%   94.67%   +<.01%     
==========================================
  Files         318      318              
  Lines        5380     5386       +6     
  Branches      112      216     +104     
==========================================
+ Hits         5093     5099       +6     
  Misses        287      287

Impacted Files	Coverage Δ
...a/cats/laws/discipline/CommutativeArrowTests.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/arrow/Arrow.scala	`100% <100%> (ø)`	⬆️
...c/main/scala/cats/laws/discipline/ArrowTests.scala	`100% <100%> (ø)`	⬆️
laws/src/main/scala/cats/laws/ArrowLaws.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/IorT.scala	`97.39% <0%> (-0.03%)`	⬇️
core/src/main/scala/cats/MonoidK.scala	`80% <0%> (ø)`	⬆️
core/src/main/scala/cats/Alternative.scala	`88.88% <0%> (ø)`	⬆️
core/src/main/scala/cats/SemigroupK.scala	`100% <0%> (ø)`	⬆️
...n/scala/cats/laws/discipline/MonadErrorTests.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/MonadError.scala	`100% <0%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c51fe61...4caa33e. Read the comment docs.

kailuowang · 2017-12-06T02:46:49Z

core/src/main/scala/cats/arrow/Arrow.scala

+
+  /**
+    * Create a new computation `F` that apply f andThen biFork the result
+    * one way is applied to g and the other pass through


This description kind of implies that f is only applied once, but actually it is applied twice right? the diagram is more accurate.

kailuowang · 2017-12-06T02:47:25Z

This merge is basically Semigroupal.product, if you just care about Function or Kleisli they already have Semigroupal instance
What this proves, though, is that Arrow forms a Semigroupal
We could add the following to Semigroupal's companion

  implicit def catsSemigroupalForArrow[F[_, _], A](implicit F: Arrow[F]): Semigroupal[F[A, ?]] = new Semigroupal[F[A, ?]] {
    def product[B, C](fb: F[A, B], fc: F[A, C]): F[A, (B, C)] =
      F.andThen(F.lift((x: A) => (x, x)), F.split(fb, fc))
  }

let me punt on if we should add this though.

kailuowang · 2017-12-06T13:51:30Z

I think this PR is good as is. I thought about renaming merge to product but I think it might cause syntax conflicts between ArrowSyntax and SemigroupalSyntax.
We can add the Semigroupal instance for F[_, ?] with Arrow in a followup PR after this one is merged.

…n in doc was not visibile in browser. Deleted the last two rows of merge method scala doc: the schema is clear enough.

ceedubs · 2017-12-06T16:32:03Z

core/src/main/scala/cats/arrow/Arrow.scala

+    *
+    */
+  @simulacrum.op("-<", alias = true)
+  def combineAndByPass[A, B, C](f: F[A, B], g: F[B, C]): F[A, (B, C)] = {


Could you explain the meaning behind this name? Should it be Bi instead of By? It makes me think of the word bypass, but I suspect you weren't going for that based on the camelCasing.

Sorry it should be 'bypass' without camelCase. I'm converting the camel to a dromedary ;-) and the I'll commit.

ceedubs · 2017-12-06T16:40:00Z

Thanks @marcobattaglia!

I definitely like having the merge, even if we decide to separately define a Semigroupal for all arrows. The use-cases for the -< method seem a little more narrow to me, but my only concerns about it are the textual name (see comment above), and potential bloating of type class methods. If people find it to be useful, I have no objections.

marcobattaglia · 2017-12-06T17:26:07Z

@ceedubs
We have often used these methods in our projects, and it happens to compose functions using -< .
We have also same methods to compose 'Kleisli arrows' but I need to study better cats in order to decide if it makes sense to prepare a PR for those . Thank you for your support and consideration.

having 2nd thought.

kailuowang · 2017-12-06T19:48:09Z

Actually, before I sign off this PR, I start to wonder what would be the difference between
calling combineAndBypass(f, g) and calling merge(f, f >>> g)?

ceedubs · 2017-12-06T21:49:42Z

core/src/main/scala/cats/arrow/Arrow.scala

+    */
+  @simulacrum.op("-<", alias = true)
+  def combineAndBypass[A, B, C](f: F[A, B], g: F[B, C]): F[A, (B, C)] = {
+    andThen(lift((x: A) => (x, x)), split(f, andThen(f, g)))


One downside of this is that it calculates f(x) twice. I think that for pretty much any arrow you would want to override this to reuse the result of f(x).

I agree. I think I incline to not adding this one, and the default impl is just an alias to merge(f, f >>> g)

Yes you are right. It is also the same of this one:
andThen(andThen( f, lift((x: B) => (x, x)) ), split(id, g))
This last one evicts the double calculation.
Essentially: input=> f(input) => out1=> (out1,out1)=> (out1,g(out1))=>(out1,out2)
Please consider if it's better to eliminate combineAndBypass because it is possible to use merge(f, f >>> g) or if it is good to have combineAndBypass as a sort of shortcut.
I'll prepare the commit following your decision.

mmm.... I think that also merge(f, f >>> g) evaluates f twice, instead this one andThen(andThen( f, lift((x: B) => (x, x)) ), split(id, g)) evaluates f one time, so that it is not properly a simple shortcut althought the result is the same.

kailuowang · 2017-12-07T02:34:40Z

core/src/main/scala/cats/arrow/Arrow.scala

+    *
+    */
+  @simulacrum.op("-<", alias = true)
+  def combineAndBypass[A, B, C](f: F[A, B], g: F[B, C]): F[A, (B, C)] = {


This can be andThen(f, merge (id, g)) right?

Right. Do you prefer to eliminate the combineAndBypass or to leave it as a simple shortcut? For me it's the same, I think that it depends on how cats is generally organized. I will commit following your opinion.

I've tries to rewrite andThe(f, merge(d,g)) as something like andThen(f, catsSemigroupalForArrow.product(id,g)) and I've noticed that catscatsSemigroupalForArrow.product is a little bit different than merge infact the product change also the types of the resulting function.
For example the type of
catsSemigroupalForArrow.product( ((x:Int) => x * 2d), ((x:Double) => x * 5) ) is Double with Int => (Double, Double) , instead the type of `((x:Int) => x * 2d) merge ((x:Int) => x * 5)' is Int => (Double, Int). My question is: 'product' means also an operation on types, in the sense that it also work on the domain types of the function ?

Your catsSemigroupalForArrow.product( ((x:Int) => x * 2d), ((x:Double) => x * 5) ) is supprising. I didn't expect that to compile. We can discuss that #2078

If you don't mind I'd prefer removing combineAndBypass. It's not providing much, typing f >>> (id &&& g) isn't too much worse than f -< g which arguably is more obscure to read. And it's a lot harder to remove a method than adding one.

I agree. I will prepare the commit to delete it. Thx

this capanion object: object Arrow { implicit def catsSemigroupalForArrow[F[_, _], A](implicit F: Arrow[F]): Semigroupal[F[A, ?]] = new Apply[F[A, ?]] { def map[B, C](fb: F[A, B])(f: B => C): F[A, C] = F.rmap(fb)(f) def ap[B, C](ff: F[A, B => C])(fb: F[A, B]): F[A, C] = F.rmap(F.andThen(F.lift((x: A) => (x, x)), F.split(ff, fb)))(tup => tup._1(tup._2)) } compiles.

…on remains

kailuowang · 2017-12-07T21:43:42Z

core/src/main/scala/cats/arrow/Arrow.scala

+    *
+    * In the reference articles "Arrows are Promiscuous...", and in the corresponding Haskell
+    * library `Control.Arrow`, this function is called `arr`.
+    */


I hate to nitpick on this, but this format is unnecessarily changed away from the scala doc convention.

Excuse me kailuowang, but I don't understand. What format is changed away? I don't know where I must correct the format, because I don't notice any difference.Please show me better.thx

The indentations of the * :)

again, it's a total nitpick. I think it's ready to merge.

marcobattaglia · 2017-12-08T13:48:50Z

Ops! Space! Sorry, I've committed just now.

kailuowang

Thanks so much!

marcobattaglia and others added 9 commits December 5, 2017 02:31

methods merge and -< work fine, but test no!

28fe55a

fix tests

c2e451f

ArrowLaws without parens around A in IsEq[F[A, (B, C)]]

29c878c

Merge pull request #1 from kailuowang/marco

4d88b6c

fix tests

added law and test for Arrow.combineAndByPass

fa6aa46

Added two method to compose Arrows 'merge' and '-<'. See comment in …

4f1c522

…cats.arrow.Arrow

Merge branch 'master' of https://github.com/marcobattaglia/cats

d7d0d47

Merge branch 'master' of https://github.com/marcobattaglia/cats

5f1e1d6

Merge branch 'master' of https://github.com/marcobattaglia/cats

f80bcb5

added exclude [ReverseMissingMethodProblem]s for Arrow.merge, Arrow.c…

87ff6a6

…ombineAndByPass and relative syntax Ops

kailuowang added this to the 1.0.0 milestone Dec 5, 2017

solved doctest errors

db2d224

kailuowang mentioned this pull request Dec 6, 2017

update Symbol table in faq #2066

Closed

kailuowang reviewed Dec 6, 2017

View reviewed changes

kailuowang previously approved these changes Dec 6, 2017

View reviewed changes

Better format in scala doc html presentation. Schema of merge functio…

2c0bc19

…n in doc was not visibile in browser. Deleted the last two rows of merge method scala doc: the schema is clear enough.

marcobattaglia dismissed kailuowang’s stale review via 2c0bc19 December 6, 2017 13:57

kailuowang previously approved these changes Dec 6, 2017

View reviewed changes

kailuowang mentioned this pull request Dec 6, 2017

Get Semigroupal from Arrow for free #2069

Closed

LukaJCB previously approved these changes Dec 6, 2017

View reviewed changes

ceedubs reviewed Dec 6, 2017

View reviewed changes

combineAndByPass has been renamed to combineAndBypass

817831a

marcobattaglia dismissed stale reviews from LukaJCB and kailuowang via 817831a December 6, 2017 17:16

kailuowang previously approved these changes Dec 6, 2017

View reviewed changes

ceedubs reviewed Dec 6, 2017

View reviewed changes

marcobattaglia added 4 commits December 7, 2017 01:18

Arrow.combineAndBypass now calculates f only one time instead twice

91dfe26

Arrow.combineAndBypass now calculates f only one time instead twice

17c6a9e

Merge branch 'master' of https://github.com/marcobattaglia/cats

c53e368

deleted one line of comment

ef5996f

kailuowang reviewed Dec 7, 2017

View reviewed changes

the combineAndBypass '-<' function has been deleted. the merge functi…

5bd2889

…on remains

kailuowang reviewed Dec 7, 2017

View reviewed changes

kailuowang changed the title ~~Added two methods to Arrow for arrows composition~~ Added merge (product) to Arrow for arrows composition Dec 8, 2017

kailuowang previously approved these changes Dec 8, 2017

View reviewed changes

space margin of comment reformatted in Arrow as standard

4caa33e

marcobattaglia dismissed kailuowang’s stale review via 4caa33e December 8, 2017 13:46

LukaJCB approved these changes Dec 8, 2017

View reviewed changes

kailuowang approved these changes Dec 8, 2017

View reviewed changes

kailuowang merged commit cb79bee into typelevel:master Dec 8, 2017

kailuowang added the enhancement label Dec 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `merge` (product) to `Arrow` for arrows composition #2063

Added `merge` (product) to `Arrow` for arrows composition #2063

marcobattaglia commented Dec 5, 2017 •

edited by kailuowang

Loading

kailuowang commented Dec 5, 2017

kailuowang commented Dec 5, 2017

marcobattaglia commented Dec 5, 2017

kailuowang commented Dec 5, 2017

kailuowang commented Dec 5, 2017 •

edited

Loading

codecov-io commented Dec 5, 2017 •

edited

Loading

kailuowang Dec 6, 2017 •

edited

Loading

kailuowang commented Dec 6, 2017 •

edited

Loading

kailuowang commented Dec 6, 2017

ceedubs Dec 6, 2017

marcobattaglia Dec 6, 2017

ceedubs commented Dec 6, 2017

marcobattaglia commented Dec 6, 2017

kailuowang commented Dec 6, 2017

ceedubs Dec 6, 2017

kailuowang Dec 6, 2017 •

edited

Loading

marcobattaglia Dec 6, 2017

marcobattaglia Dec 6, 2017

kailuowang Dec 7, 2017

marcobattaglia Dec 7, 2017

marcobattaglia Dec 7, 2017

kailuowang Dec 7, 2017

marcobattaglia Dec 7, 2017

marcobattaglia Dec 7, 2017

kailuowang Dec 7, 2017

marcobattaglia Dec 7, 2017

kailuowang Dec 7, 2017

kailuowang Dec 8, 2017

marcobattaglia commented Dec 8, 2017

kailuowang left a comment

Added merge (product) to Arrow for arrows composition #2063

Added merge (product) to Arrow for arrows composition #2063

Conversation

marcobattaglia commented Dec 5, 2017 • edited by kailuowang Loading

kailuowang commented Dec 5, 2017

kailuowang commented Dec 5, 2017

marcobattaglia commented Dec 5, 2017

kailuowang commented Dec 5, 2017

kailuowang commented Dec 5, 2017 • edited Loading

codecov-io commented Dec 5, 2017 • edited Loading

Codecov Report

kailuowang Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang commented Dec 6, 2017 • edited Loading

kailuowang commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceedubs commented Dec 6, 2017

marcobattaglia commented Dec 6, 2017

kailuowang commented Dec 6, 2017

Choose a reason for hiding this comment

kailuowang Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcobattaglia commented Dec 8, 2017

kailuowang left a comment

Choose a reason for hiding this comment

Added `merge` (product) to `Arrow` for arrows composition #2063

Added `merge` (product) to `Arrow` for arrows composition #2063

marcobattaglia commented Dec 5, 2017 •

edited by kailuowang

Loading

kailuowang commented Dec 5, 2017 •

edited

Loading

codecov-io commented Dec 5, 2017 •

edited

Loading

kailuowang Dec 6, 2017 •

edited

Loading

kailuowang commented Dec 6, 2017 •

edited

Loading

kailuowang Dec 6, 2017 •

edited

Loading