SketchMap doesn't use Implicit Monoid on V #152

Merged
merged 2 commits into from Mar 27, 2013

Conversation

Projects
None yet
3 participants
Contributor

wlue commented Mar 26, 2013

SketchMap doesn't properly use the implicit monoid.

This pull request fixes this issue. Added a test to verify.

@sritchie sritchie and 2 others commented on an outdated diff Mar 26, 2013

...e/src/main/scala/com/twitter/algebird/SketchMap.scala
@@ -138,6 +138,12 @@ case class SketchMap[K, V](
)(implicit ordering: Ordering[V], monoid: Monoid[V]) extends java.io.Serializable {
/**
+ * Redefine the AdaptiveMatrix Monoid so it takes the implicit Monoid from
+ * the SketchMap.
+ */
+ private implicit val valuesTableMonoid: Monoid[AdaptiveMatrix[V]] = AdaptiveMatrix.monoid[V]
@sritchie

sritchie Mar 26, 2013

Collaborator

It's actually weird that this doesn't happen by default. I wonder if this is hurting us elsewhere.

@johnynek

johnynek Mar 26, 2013

Collaborator

How do you know this fixes your issue? I'm very confused as to how this is doing anything. You are redefining the same implicit monoid.

Can you provide a gist of a failed test without this and explain the mechanism? This looks like we are hacking around a compiler bug at best. This is not how implicit resolution works, as far as I can understand.

@wlue

wlue Mar 27, 2013

Contributor

Looks like my failed test was due to other silly reasons, it doesn't seem to be a problem. (the test passes without that line). I'll update the diff to remove the change, since the test is still useful.

Collaborator

johnynek commented Mar 27, 2013

Looks like your tests still passed right (the one that failed looks like it something going on with a flaky test or framework issues).

Contributor

wlue commented Mar 27, 2013

Yeah, I initially wrote the test incorrectly, which made me assume that the change I made with the implicit fixed it. The test is still good though.

@johnynek johnynek added a commit that referenced this pull request Mar 27, 2013

@johnynek johnynek Merge pull request #152 from twitter/fix/sketchmap-monoid
SketchMap doesn't use Implicit Monoid on V
7cc1344

@johnynek johnynek merged commit 7cc1344 into develop Mar 27, 2013

1 check failed

default The Travis build failed
Details

johnynek deleted the fix/sketchmap-monoid branch Mar 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment