Simple Fix of SketchMap's non-commutative (issue-1122) #1123
Conversation
|
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #1123 +/- ##
===========================================
+ Coverage 89.48% 89.72% +0.23%
===========================================
Files 124 124
Lines 10028 10030 +2
Branches 782 774 -8
===========================================
+ Hits 8974 8999 +25
+ Misses 1054 1031 -23
☔ View full report in Codecov by Sentry. |
| val newValuesTable = right.valuesTable match { | ||
| case DenseMatrix(_, _, _) => Monoid.plus(right.valuesTable, left.valuesTable) | ||
| case _ => Monoid.plus(left.valuesTable, right.valuesTable) | ||
| } |
There was a problem hiding this comment.
| val newValuesTable = right.valuesTable match { | |
| case DenseMatrix(_, _, _) => Monoid.plus(right.valuesTable, left.valuesTable) | |
| case _ => Monoid.plus(left.valuesTable, right.valuesTable) | |
| } | |
| val newValuesTable = right.valuesTable match { | |
| case _: DenseMatrix[V] => Monoid.plus(right.valuesTable, left.valuesTable) | |
| case _ => Monoid.plus(left.valuesTable, right.valuesTable) | |
| } |
Also, perhaps it would make more sense to address this here
There was a problem hiding this comment.
Also, perhaps it would make more sense to address this here
yeah, pretty much agreed.
I was afraid to touch AdaptiveMatrix due to unfamiliarity of it, but will try to see if I can fix that directly.
| @@ -53,6 +55,38 @@ class SketchMapTest extends AnyWordSpec with Matchers { | |||
| assert(sm.totalValue == totalCount) | |||
| } | |||
|
|
|||
| "plus should work commutatively" in { | |||
| implicit val m = Monoid.longMonoid | |||
There was a problem hiding this comment.
I believe this is not required
This PR introduces a simple fix to SketchMap's non-commutative, as illustrated in #1122
The fix is simply to always put the DenseMatrix as the first param so that the update to it is correct.