Adding NonEmptyVector #1137

zainab-ali · 2016-06-17T15:07:58Z

Adding NonEmptyVector as discussed in #1088

ceedubs · 2016-06-17T15:23:11Z

@zainab-ali thanks for working on this!

It looks like there are already merge conflicts, so you'll want to pull in the latest changes from master.

Also please note the second paragraph of the description for #1088 -- I think we'll want to change this implementation to not use the "cons" approach for Vector.

kailuowang · 2016-06-17T17:21:14Z

core/src/main/scala/cats/data/package.scala

@@ -11,11 +10,6 @@ package object data {
  def NonEmptyList[A](head: A, tail: A*): NonEmptyList[A] =
    OneAnd[List, A](head, tail.toList)

-  def NonEmptyVector[A](head: A, tail: Vector[A] = Vector.empty): NonEmptyVector[A] =
-    OneAnd(head, tail)
-  def NonEmptyVector[A](head: A, tail: A*): NonEmptyVector[A] =


IMO, this API (head : A, tail: A*) would be handy.

zainab-ali · 2016-06-17T22:53:25Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+   */
+  def show(implicit A: Show[A]): String =
+    s"NonEmptyVector(${A.show(head)}, ${Show[Vector[A]].show(tail)})"
+}


Is this show correct?
Or should I have s"NonEmptyVector(${Show[Vector[A]].show(vector)})" instead?

I think it's better than the cons like show.

codecov-io · 2016-06-18T00:05:20Z

Current coverage is 89.09%

Merging #1137 into master will increase coverage by 0.29%

@@             master      #1137   diff @@
==========================================
  Files           232        234     +2   
  Lines          3073       3137    +64   
  Methods        3021       3082    +61   
  Messages          0          0          
  Branches         49         52     +3   
==========================================
+ Hits           2729       2795    +66   
+ Misses          344        342     -2   
  Partials          0          0

Powered by Codecov. Last updated by a639f66...e173928

non · 2016-06-18T03:48:34Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+
+object NonEmptyVector extends NonEmptyVectorInstances {
+
+  def apply[A](head: A, tail: Vector[A]): NonEmptyVector[A] = NonEmptyVector(head +: tail)


I would prefer to support this as:

def apply[A](body: Vector[A], last: A): NonEmptyVector[A] = NonEmptyVector(body :+ last)

In general I don't like treating non-empty vector as a head/tail pair because it's so inefficient -- prepending to a vector is O(n) and reallocates the entire vector. Appending is a lot more efficient, so in this case I think it makes more sense.

Honestly I'd probably prefer an interface like:

def apply[A](xs: Vector[A]): Option[NonEmptyVector[A]] = ...

I probably missed something. I have been holding the belief that according to
http://docs.scala-lang.org/overviews/collections/performance-characteristics.html
Prepending to a vector is as efficient as appending? I thought it makes sense based on my (not very thorough) understanding of the implementation of vector. Is there some caveat in this claim?

I might propose the following instantiation methods:

// to be consistent with var-arg `apply` methods for other collections def apply[A](head: A, tail: A*): NonEmptyVector[A] def fromVector[A](vector: Vector[A]): Option[NonEmptyVector[A]] // this will throw an exception if the vector is empty def fromVectorUnsafe[A](vector: Vector[A]): NonEmptyVector[A] def prepend[A](head: A, tail: Vector[A]): NonEmptyVector[A] def append[A](vector: Vector[A], last: A): NonEmptyVector[A]

I think that would cover all of the use-cases. WDYT?

You're right @kailuowang. I forgot that vectors maintain a start index to allow this kind of thing to be effectively constant time. I was wrong.

I think maybe I got confused because for awhile Vector(x) ++ xs was very slow. But this was just a bug, not related to x +: xs.

ceedubs · 2016-06-18T13:04:53Z

@zainab-ali thanks, this is getting really close I think!

johnynek · 2016-06-18T20:46:01Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+    */
+  def flatMap[B](f: A => NonEmptyVector[B]): NonEmptyVector[B] =
+    NonEmptyVector(vector.flatMap(a => f(a).vector))
+


what about reduce? Since, we know that this is non-empty the reduce(fn: (A, A) => A): A is a safe signature. That or we could have: def combineAll(implicit: sg: Semigroup[T]): T (or we could have both).

reduce and several other methods are available via the Reducible instance, but I agree that it would probably be nice to have some of these helpers on the class itself.

I think they should be called reduceLeft(f: (A, A) => A): A and reduce(implicit S: Semigroup[A]): A to be consistent with cats.Reducible. AFAK, the standard library has similar names.

zainab-ali · 2016-06-27T13:55:03Z

I've been mulling over the naming conventions for safe / unsafe methods.

I agree with the following conventions:

If a function is unsafe, it should be suffixed by unsafe. This is because cats is safe by default.
We should stick to the standard library conventions whenever they are safe.

So when a function is safe, but is analogous to an unsafe standard library function, what should we do?
e.g updated(i: Int): Option[A], apply(i: Int): Option[A], Vector.apply(v: Vector[A]): Option[NonEmptyVector[A]]

Since cats is safe by default, we could keep the names as is, but as @johnynek points out, is causes confusion for users of the standard library, when functions they expect to be unsafe start returning options.

In the above comments, we've decided rename the functions instead:
updated to updatedOption
apply(i: Int) to get, and the unsafe variant to getUnsafe
apply(v: Vector[A]) to fromVector and the unsafe variant to fromVectorUnsafe

TBH, I'm uneasy about this, because I find variation in method names more confusing than variation in signatures. However, since I can't think of a better solution, I'll go with it.

We should use the same convention for the NonEmptyList in #1120

johnynek · 2016-06-27T19:48:18Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+    * Left-associative reduce using the Semigroup of A
+    */
+  def reduce(implicit S: Semigroup[A]): A =
+    reduceLeft(S.combine)


semigroup has combineOption which can possibly be faster than using combine.

ceedubs · 2016-07-01T10:37:51Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+
+  /** Gets the element at the index, if it exists */
+  def get(i: Int): Option[A] =
+    Try(getUnsafe(i)).toOption


I think that toVector.lift(i) would probably be a more efficient implementation (though this is performance speculation without a benchmark).

ceedubs · 2016-07-01T10:43:44Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+
+  /** Updates the element at the index, if it exists */
+  def updated(i: Int, a: A): Option[NonEmptyVector[A]] =
+    Try(updatedUnsafe(i, a)).toOption


More performance speculation: if (toVector.isDefinedAt(i)) Some(toVector.updated(i, a)) else None.

Oops I guess that would need to wrap it in a NonEmptyVector too.

ceedubs · 2016-07-01T11:20:46Z

👍 thanks @zainab-ali!

I've left some minor comments about possibly more performant implementations of a few things, but I'm happy to let those happen in a followup PR.

johnynek · 2016-07-01T18:13:48Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+    * Left-associative reduce using the Semigroup of A
+    */
+  def reduce(implicit S: Semigroup[A]): A =
+    S.combineAllOption(tail).foldLeft(head)(S.combine)


personally I would do: S.combineAllOption(toVector).get since we know the get can't throw since toVector.nonEmpty.

kailuowang · 2016-07-11T17:45:29Z

👍

peterneyens · 2016-07-11T18:09:32Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+   * universal .toString method.
+   */
+  def show(implicit A: Show[A]): String =
+    s"NonEmptyVector(${Show[Vector[A]].show(toVector)})"


I think this will give us something like NonEmptyVector(Vector(1, 2, 3)) for NonEmptyVector(1, 2, 3).
Maybe we could do "NonEmpty" + Show[Vector[A]].show(toVector) which should give us NonEmptyVector(1,2,3) ?

peterneyens · 2016-07-11T18:47:21Z

This looks great @zainab-ali, I left some minor comments.

Another thing which might be useful is an unapply method on the NonEmptyVector object so you can pattern match on NonEmptyVector, but this can probably be added in a later PR.

ceedubs · 2016-07-13T11:39:28Z

core/src/main/scala/cats/data/NonEmptyVector.scala

+  def concat(other: NonEmptyVector[A]): NonEmptyVector[A] = NonEmptyVector(toVector ++ other.toVector)
+
+  /**
+    * Alias for concat


Would be nice to make this Alias for [[concat]] so that the scaladoc has a link.

Oh I see that we are using overloading, so I don't know how that works with scaladoc. I'm generally skeptical of overloading, though it's probably fine in this instance. I think it would prevent us from being able to turn NonEmptyVector into a value class though, wouldn't it?

non · 2016-07-13T21:39:43Z

It looks we have have multiple thumbs-up. I'm going to give mine too 👍 and merge this. There is probably some follow-up work to do, but let's open a new PR for that.

This follows up on several small suggestions from comments in typelevel#1137. The only meaningful modification is changing the format of `show` from something like `NonEmptyVector(Vector(1, 2, 3))` to something like `NonEmptyVector(1, 2, 3)`. I've also added a `length` method to `NonEmptyVector`.

Adding NonEmptyVector

c6c8ee0

zainab-ali mentioned this pull request Jun 17, 2016

create cats.data.NonEmptyVector #1088

Closed

zainab-ali added 2 commits June 17, 2016 16:31

Optimising NonEmptyVector

fb7988f

Merge branch 'master' into vector1

b715ee7

kailuowang reviewed Jun 17, 2016
View reviewed changes

Adding NonEmptyVector

74e8a40

zainab-ali reviewed Jun 17, 2016
View reviewed changes

Altering Show for NonEmptyVector

33b845f

non reviewed Jun 18, 2016
View reviewed changes

johnynek reviewed Jun 18, 2016
View reviewed changes

Additional methods

d237bdb

johnynek reviewed Jun 27, 2016
View reviewed changes

zainab-ali added 4 commits June 29, 2016 19:05

Adding MonadRec to NonEmptyVector

a79bc57

Merge remote-tracking branch 'upstream/master' into vector1

a9ceb60

Adding concat aliases to NonEmptyVector

e35ada2

Fixing tailRecM on NonEmptyVector

696e269

ceedubs reviewed Jul 1, 2016
View reviewed changes

johnynek reviewed Jul 1, 2016
View reviewed changes

Adding more tests to NonEmptyVector

e173928

peterneyens reviewed Jul 11, 2016
View reviewed changes

ceedubs reviewed Jul 13, 2016
View reviewed changes

non merged commit 556e97b into typelevel:master Jul 13, 2016

ceedubs mentioned this pull request Jul 15, 2016

Change format of NonEmptyVector.show #1203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding NonEmptyVector #1137

Adding NonEmptyVector #1137

zainab-ali commented Jun 17, 2016

ceedubs commented Jun 17, 2016

kailuowang Jun 17, 2016 •

edited

Loading

zainab-ali Jun 17, 2016

kailuowang Jun 17, 2016

codecov-io commented Jun 18, 2016 •

edited

Loading

non Jun 18, 2016 •

edited

Loading

kailuowang Jun 18, 2016

ceedubs Jun 18, 2016

non Jun 18, 2016

ceedubs commented Jun 18, 2016

johnynek Jun 18, 2016

ceedubs Jun 19, 2016

zainab-ali Jun 27, 2016

zainab-ali commented Jun 27, 2016

johnynek Jun 27, 2016

ceedubs Jul 1, 2016

ceedubs Jul 1, 2016

ceedubs Jul 1, 2016

ceedubs commented Jul 1, 2016

johnynek Jul 1, 2016

kailuowang commented Jul 11, 2016

peterneyens Jul 11, 2016

peterneyens commented Jul 11, 2016

ceedubs Jul 13, 2016

ceedubs Jul 13, 2016

non commented Jul 13, 2016


		object NonEmptyVector extends NonEmptyVectorInstances {

		def apply[A](head: A, tail: Vector[A]): NonEmptyVector[A] = NonEmptyVector(head +: tail)

Adding NonEmptyVector #1137

Adding NonEmptyVector #1137

Conversation

zainab-ali commented Jun 17, 2016

ceedubs commented Jun 17, 2016

kailuowang Jun 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 18, 2016 • edited Loading

Current coverage is 89.09%

non Jun 18, 2016 • 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

ceedubs commented Jun 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zainab-ali commented Jun 27, 2016

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

ceedubs commented Jul 1, 2016

Choose a reason for hiding this comment

kailuowang commented Jul 11, 2016

Choose a reason for hiding this comment

peterneyens commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

non commented Jul 13, 2016

kailuowang Jun 17, 2016 •

edited

Loading

codecov-io commented Jun 18, 2016 •

edited

Loading

non Jun 18, 2016 •

edited

Loading