Mio/issue 583

[mio-stripe]

Fixing issue 583 that AdaptiveVector monoid fails monoid laws when the sparseValue is monoid.zero.

[codecov-io]

Codecov Report

Merging #629 into develop will decrease coverage by 0.12%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop     #629      +/-   ##
===========================================
- Coverage    82.07%   81.95%   -0.13%     
===========================================
  Files          110      111       +1     
  Lines         5155     5175      +20     
  Branches       457      470      +13     
===========================================
+ Hits          4231     4241      +10     
- Misses         924      934      +10

Impacted Files	Coverage Δ
...in/scala/com/twitter/algebird/AdaptiveVector.scala	77.87% <83.33%> (-4.04%)	⬇️
.../scala/com/twitter/algebird/MetricProperties.scala	0% <0%> (-100%)	⬇️
.../main/scala/com/twitter/algebird/Applicative.scala	61.29% <0%> (-3.23%)	⬇️
.../main/scala/com/twitter/algebird/BloomFilter.scala	93.01% <0%> (-0.88%)	⬇️
.../main/scala/com/twitter/algebird/Successible.scala	91.66% <0%> (ø)	⬆️
...src/main/scala/com/twitter/algebird/Interval.scala	64.28% <0%> (ø)	⬆️
.../scala/com/twitter/algebird/MetricProperties.scala	100% <0%> (ø)
...in/scala/com/twitter/algebird/scalacheck/Gen.scala	100% <0%> (+8.33%)	⬆️

---

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 04a9adc...926abd1. Read the comment docs.

[johnynek]

Thanks for doing this! very helpful.

A few minor comments.

[johnynek]

don't need to capture sem. Semgroup.plus(a, a) would work.

[mio-stripe]

👍

[johnynek]

what is up with the IS?

[mio-stripe]

I'll change these, I got into doing that as a note-to-self.

[johnynek]

let's not comment out algebirdSpark. It is a pain that 2.12.x won't build, but CI knows about it. Can build with sbt ++2.11.8 compile on the command line.

[mio-stripe]

Will change, didn't mean to commit this. Cool, I'll build that way.

[mio-stripe]

So, what I discovered is that this is a bit confusing because the algebra of sparse values is entwined with the algebra of values you care about. The clean solution to this would be to make this AdpativeVector[V, SV] where the type of the sparse value is distinct from the type of the values, then be able to define the algebra of sparse values however you want. This way, AVSemigroup.plus could add sparse values or not depending on your semigroup structure on SV. This is not what I did, but I'm happy to make that generalization if there is interest. What I did is update plus and equiv so that

• adding AdpativeVectors does not add their sparseValues

• explicitly check if a summand to AVSemigroup.plus is AVMonoid.zero or if it is a zero vector so that both AVMonoid.zero and other zero vectors all act as additive identities

• came up what I think is a reasonable definition of equiv

• got all the tests to pass except the AdaptiveVector[Int] is a group (when sparseValue is 1) because the group structure for AdaptiveVector relies on adding the sparseValues.
  I changed this test to check the group properties when the sparse value is 0 and it passes.

[johnynek]

@mio-stripe is this ready to merge? Seems like it might be?

No longer WIP?

[johnynek]

this looks good, but now I'm worried that the plus is so expensive we are killing any optimizations that this class intends to represent.

Maybe we should write a benchmark in the algebird-benchmarks and compare Vector[T] with AdaptiveVector[T] when we have a lot of sparsity.

[mio-stripe]

@johnynek ready to merge! Can we add the benchmark after or should we do it before merging this?

[johnynek]

I needed to publish a version. I don't want to publish this without a benchmark, so I'm going to keep this open until we have the benchmark. I can find time to pair with you if you want sometime soon.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}