scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

TracersableOnce[T].sum requires Numeric, which is too speific requirement #9075

Open scabug opened 9 years ago

scabug commented 9 years ago

In Scala 2.11, the TracersableOnce[T].sum method is defined as follows:

def sum[B >: A](implicit num: Numeric[B]): B = foldLeft(num.zero)(num.plus)

This might look well until you try to sum, say, durations. Summing durations is pretty legal, since there is a defined associative operation plus and there is a zero element, which is all what is needed in the sum method. However, scala.math.Numeric[T] requires to also implement some other methods.

This is not just a theoretical restriction, because I can't properly implement (_: Duration).times(_: Duration): Duration. When multiplying time duration by another time duration, I get a "square duration", whatever it means. As a result, I can either use an invalid implementation (e.g. def times(d: Duration) = ???) or I can completely resign on TraversableOnce[Duration].sum. None of these is ideal.

I suggest creating a more fine-grained hierarchy, e.g. scala.math.Monoid[T]. Requiring just a monoid seems to be enough for the sum method and removes the insane requirements.

scabug commented 9 years ago

Imported From: https://issues.scala-lang.org/browse/SI-9075?orig=1 Reporter: Vít Šesták (v6ak) Affected Versions: 2.11.4

scabug commented 9 years ago

@soc said (edited on Jan 10, 2015 10:23:24 PM UTC): I'm sympathetic, but this would turn code which was {code}O(1){code} into {code}O(n){code}.

scabug commented 9 years ago

Vít Šesták (v6ak) said: Are you sure? The original implementation with foldLeft is {code}O(n){code} and this change does not change its complexity.

Well, there are some exceptions like Range.sum, which is optimized under some conditions to run in {code}O(1){code}, but this generalization would not affect it in any important way. (Well, Range.sum might have to be slightly rewritten in order to preserve the optimization in all cases.)

scabug commented 9 years ago

@soc said: How would you rewrite Range.sum with only a Monoid, preserving the current performance characteristics?

scabug commented 9 years ago

Vít Šesták (v6ak) said: Look at the current implementation: https://github.com/scala/scala/blob/1620b8cf772f0c4371220e05c54a3e3c5ccab349/src/library/scala/collection/immutable/Range.scala#L356-L375

The optimized branch does not use Numeric[B], but rather directly Int methods. Nothing is to be changed there.

The generic (non-optimized) branch, well, uses zero, plus and toInt. In this case, we maybe need a NumericMonoid that also provides toInt. But we don't need all the power of Numeric trait.

scabug commented 9 years ago

@Ichoran said: I think we should seriously rework Numeric et al. in 2.13 along the lines of what Spire has done. On backlog until then, but I'm highly sympathetic to the concern.