Add size comparison methods to IterableOps #6950

NthPortal · 2018-07-18T06:14:16Z

Add IterableOps.sizeCompare(Int), .sizeCompare(Iterable[_]), and
.sizeIs

Addresses this comment on scala/collection-strawman#338

NthPortal · 2018-07-18T06:14:56Z

src/library/scala/collection/Iterable.scala

+    *  is `O(size min _size)` instead of `O(size)`. The method should be overwritten
+    *  if computing `size` is cheap.
+    */
+  def sizeCompare(_size: Int): Int = {


not super happy with this parameter name, but I wasn't sure what else to call it, as shadowing the size method would be confusing

Maybe otherSize or thatSize?

NthPortal · 2018-07-18T06:16:39Z

src/library/scala/collection/Iterable.scala

+    * this.sizeIs > size     // this.sizeCompare(size) > 0
+    * }}}
+    */
+  @inline final def sizeIs: IterableOps.SizeCompareOps = new IterableOps.SizeCompareOps(this)


not sure if lengthIs should just alias to this, or what. The issue is, then the documentation for all of its ops would say "size" instead of "length"; I'm not sure if that's an issue.

As it is right now, IterableOps.SizeCompareOps and SeqOps.LengthCompareOps are pretty much the same

NthPortal · 2018-07-18T06:25:04Z

Yes, this needs at least a test for compareSizesWith, as well as perhaps some test reorganization. However, I wanted to push the PR up to allow for feedback on the changes as soon as possible

dwijnand · 2018-07-18T08:09:16Z

If the results of sizeCompare and compareSizesWith are to be tested with being less/greater than and/or equal to 0, why not have their type be a little LT/EQ/GT sum type instead of an Int?

NthPortal · 2018-07-18T08:22:33Z

@dwijnand because Java (probably) :(

sizeCompare is equivalent to lengthCompare on Seqs, and lengthCompare already returns an Int prior to 2.13, so it's a little late to change that. And it would be pretty inconsistent for just compareSizesWith to introduce a new return type.

Early on, someone decided that Ordering, Ordered, and friends should return Ints, and... sadly, I feel like it's too late to change that, as much as I'd love to.

I am 100% with you, especially since I'm pretty sure returning an Int is what leads to the mistake of trying to subtract numbers to compare them, which isn't generally valid (see #6930, for example)

dwijnand · 2018-07-18T08:36:24Z

Let's break some eggs 😄. There is the lengthCompare precedent but I'd say it shouldn't be followed. Perhaps the difference can be lived with, or perhaps it should be dealt by deprecating int-fraternizer lengthCompare?

because Java (probably) :(

Can't the Java user .equals the results? Alternatively:

the type could be a java enum rather than a sealed family (is there a precedent of shipping java enums in the standard library?)
the type could have the suite of lt/lte/.. methods on it for both Java and Scala users

NthPortal · 2018-07-18T13:19:10Z

oh, I just meant because Java decided to return an int for comparison methods. I didn't mean for Java interop

Ichoran · 2018-07-18T20:56:35Z

We don't have zero-cost abstraction of enums that work in a Scala-friendly way, so I don't think there are good options for returning a trinary that won't slow things down. Since the point of these methods is for speed, typically, slowing things down seems like a bad idea.

(Point is--ints are really fast. If you do anything else, it should be accompanied by a benchmark indicating that the difference isn't an issue. Even if it were a good idea to break with precedent, which it probably isn't.)

SethTisue · 2018-07-18T21:14:54Z

sadly, I feel like it's too late to change that, as much as I'd love to

agree

dwijnand · 2018-07-19T10:17:57Z

ints are really fast

Is int < faster than reference equality?

dwijnand · 2018-07-19T10:19:52Z

Note: my feedback shouldn't in any way be considered a blocker to this change landing. AFAIC this can land as is.

Ichoran · 2018-07-20T19:31:10Z

@dwijnand - It's a smaller factor than I thought. If we have

sealed trait Cmp {}
final object Lt extends Cmp {}
final object Eq extends Cmp {}
final object Gt extends Cmp {}

def compare(a: Int, b: Int) = if (a < b) Lt else if (a > b) Gt else Eq

then in a tight loop I measure it to be less than 5% slower than using java.lang.Integer.compare(a, b).

dwijnand · 2018-07-20T21:37:38Z

That's interesting! Thanks for sharing.

julienrf · 2018-07-21T14:06:45Z

src/library/scala/collection/Iterable.scala

+    *  is `O(size min _size)` instead of `O(size)`. The method should be overwritten
+    *  if computing `size` is cheap.
+    */
+  def sizeCompare(_size: Int): Int = {


Maybe otherSize or thatSize?

julienrf · 2018-07-21T14:13:21Z

src/library/scala/collection/Iterable.scala

+    *  is `O(this.size min that.size)` instead of `O(this.size + that.size)`.
+    *  The method should be overwritten if computing `size` is cheap.
+    */
+  def compareSizesWith(that: Iterable[_]): Int = {


What about using the singular for “size” here: compareSizeWith?

IANAG(rammarian), but I thought that "sizes" makes more sense, since I am comparing the size of this to the size of that (i.e. two sizes), rather than the size of this to that (which would be one size, and also nonsense). I am happy to be be corrected by someone that plural is not more grammatically correct in this case.

Edit: e.g. "I compared phones with my friend"

If most others thinks compareSizeWith is more natural or intuitive, I'm happy to change it

Do you feel more comfortable with compareBySizeWith?

Neither of the With ones are great because we normally use With to mean we're using a different comparator (e.g. sortWith). compareSizeTo is fine (shorthand for "compare my size to that's size"). Or, as we did with compare, we could simply drop the To--not compareTo, just compare, so not compareSizeTo but just compareSize.

The plural works too (shorthand for "compare our respective sizes"), but since the non-plural version has a sensible meaning I'd go for it instead. Having to remember random s's is annoying.

honestly, that makes me more inclined to call it sizeCompare. I think it makes more sense grammatically than compareSize (which I still opine ought to use the plural), and is also consistent with the other sizeCompare method. I think that compareSizesWith is sufficiently different from sizeCompare that one would be unlikely to mix them up, but having both sizeCompare and compareSize would quite confusing.

Overloading sizeCompare looks good to me.

julienrf · 2018-07-21T14:20:56Z

test/junit/scala/collection/IterableTest.scala

+    val seq = Seq(1, 2, 3)
+    assert(seq.compareSizesWith(Seq(1, 2)) > 0)
+    assert(seq.compareSizesWith(Seq(1, 2, 3)) == 0)
+    assert(seq.compareSizesWith(Seq(1, 2, 3, 4)) < 0)


I think we should explicitly use List here, and then write another test with a collection implementation that have a knownSize different from -1.

Add IterableOps.sizeCompare(Int), .sizeCompare(Iterable[_]), and .sizeIs

NthPortal · 2018-07-27T04:57:57Z

ping

dwijnand

Thanks Nth!

NthPortal requested review from julienrf and Ichoran July 18, 2018 06:14

scala-jenkins added this to the 2.13.0-M5 milestone Jul 18, 2018

NthPortal commented Jul 18, 2018

View reviewed changes

julienrf reviewed Jul 21, 2018

View reviewed changes

Add size comparison methods to IterableOps

68161af

Add IterableOps.sizeCompare(Int), .sizeCompare(Iterable[_]), and .sizeIs

NthPortal force-pushed the topic/sizeCompare/PR branch from 4323252 to 68161af Compare July 24, 2018 07:38

dwijnand approved these changes Jul 27, 2018

View reviewed changes

dwijnand merged commit ea9795e into scala:2.13.x Jul 27, 2018

NthPortal deleted the topic/sizeCompare/PR branch July 27, 2018 13:26

SethTisue added the release-notes worth highlighting in next release notes label Jul 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add size comparison methods to IterableOps #6950

Add size comparison methods to IterableOps #6950

NthPortal commented Jul 18, 2018 •

edited

NthPortal Jul 18, 2018

julienrf Jul 21, 2018

NthPortal Jul 18, 2018

NthPortal commented Jul 18, 2018

dwijnand commented Jul 18, 2018

NthPortal commented Jul 18, 2018

dwijnand commented Jul 18, 2018

NthPortal commented Jul 18, 2018

Ichoran commented Jul 18, 2018 •

edited

SethTisue commented Jul 18, 2018

dwijnand commented Jul 19, 2018

dwijnand commented Jul 19, 2018

Ichoran commented Jul 20, 2018

dwijnand commented Jul 20, 2018

julienrf Jul 21, 2018

julienrf Jul 21, 2018

NthPortal Jul 22, 2018 •

edited

NthPortal Jul 22, 2018

dwijnand Jul 22, 2018

Ichoran Jul 23, 2018

NthPortal Jul 24, 2018

julienrf Jul 24, 2018

julienrf Jul 21, 2018

NthPortal commented Jul 27, 2018

dwijnand left a comment

Add size comparison methods to IterableOps #6950

Add size comparison methods to IterableOps #6950

Conversation

NthPortal commented Jul 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal commented Jul 18, 2018

dwijnand commented Jul 18, 2018

NthPortal commented Jul 18, 2018

dwijnand commented Jul 18, 2018

NthPortal commented Jul 18, 2018

Ichoran commented Jul 18, 2018 • edited

SethTisue commented Jul 18, 2018

dwijnand commented Jul 19, 2018

dwijnand commented Jul 19, 2018

Ichoran commented Jul 20, 2018

dwijnand commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal Jul 22, 2018 • edited

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

NthPortal commented Jul 27, 2018

dwijnand left a comment

Choose a reason for hiding this comment

NthPortal commented Jul 18, 2018 •

edited

Ichoran commented Jul 18, 2018 •

edited

NthPortal Jul 22, 2018 •

edited