Improve test coverage for VectorsCombiner and make vector aggregator efficient #168
Merged
Improve test coverage for VectorsCombiner and make vector aggregator efficient #168
Conversation
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
==========================================
+ Coverage 80.84% 86.24% +5.39%
==========================================
Files 304 304
Lines 9920 9931 +11
Branches 334 549 +215
==========================================
+ Hits 8020 8565 +545
+ Misses 1900 1366 -534
Continue to review full report at Codecov.
|
leahmcguire
reviewed
Oct 31, 2018
| * @throws IllegalArgumentException if the vectors have different sizes | ||
| * @return vector subtraction | ||
| */ | ||
| def -(that: OPVector): OPVector = (value - that.value).toOPVector |
Collaborator
There was a problem hiding this comment.
as long as you are adding addition and subtraction why not dot product and cross product?
leahmcguire
reviewed
Oct 31, 2018
| * @return result vector | ||
| */ | ||
| def combine(that: Vector, other: Vector*): Vector = | ||
| com.salesforce.op.utils.spark.RichVector.combine(v +: that +: other) |
Collaborator
There was a problem hiding this comment.
do you need the full import here?
Collaborator
Author
There was a problem hiding this comment.
this is to distinguish the object from the implicit class.
leahmcguire
reviewed
Oct 31, 2018
| for {(v1, v2) <- vectors.zip(vectors)} { | ||
| (v1 - v2) shouldBe (v1.value - v2.value).toOPVector | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
maybe tests for the errors when the vectors have different lengths?
Collaborator
Author
|
@leahmcguire done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues
UnionVectormonoid aggregator is not efficient for sparse vectors.VectorsCombinerstage has incomplete functional test coverage.Describe the proposed solution
VectorsCombiner.combinemethod toRichVectorobjectOpEstimatorSpecbase class toVectorsCombinerTestto improve the functional test coverage+,-,dot,combinemethods toOPVector+ testOPVectorTestcombinemethod toRichVector+ testsUnionVectormonoid aggregator toCombineVectorand made use ofRichVector.combineat reduveSumVectormonoid aggregatorDescribe alternatives you've considered
I could relocate
VectorsCombiner.combineas-is toutilssubproject, but it felt valuable enough to add+,-,dotandcombinetoOPVectoras well.