Implement MagnitudeVector and add corresponding test cases - #781
Conversation
SYaoJun
changed the title
b7da49e to
5b5182e
Compare
|
@Leomrlin Could you please take a look at this code when you have a moment? |
| @Override | ||
| public double match(IVector other) { | ||
| return 0; | ||
| if (!(other instanceof MagnitudeVector)) { |
There was a problem hiding this comment.
Code snippet: if (!(other instanceof MagnitudeVector)) { throw new IllegalArgumentException("Other vector must be a MagnitudeVector"); }
Problem: This can cause runtime exceptions when calling match without explicitly performing type checking when combining multiple IVectors (EmbeddingVector, KeywordVector, TraversalVector, etc.) in VectorSearch, potentially interrupting the search process.
Recommendation: For IVectors of different types, it's advisable to return 0.0 (indicating dissimilarity/not included in the scoring), or have a more explicit contract defined by the IVector interface (e.g., match supports different types and returns 0). Returning 0 is more robust and consistent with the design comment in paste ("if not same type return 0.0").
| } | ||
|
|
||
| private double computeSimilarity(double otherMagnitude) { | ||
| if (this.magnitude == 0.0 && otherMagnitude == 0.0) { |
There was a problem hiding this comment.
In the code, "magnitude" typically refers to metrics such as degree or PageRank—values that are generally non-negative (≥0)—though certain centrality measures (e.g., specific eigenvector-based or normalized metrics) can be negative. The current implementation handles this using Math.abs, ensuring the calculation proceeds; however, it is advisable to either explicitly define the valid range (≥0) in the class documentation or formally document the meaning of negative values if they are permitted.
Recommendation: Add Javadoc to the constructor specifying the expected range, or implement validation within the constructor (throwing an IllegalArgumentException if only non-negative values are allowed).
2 participants
What changes were proposed in this pull request?
fix: #773
add MagnitudeVector with match logic and unit tests
How was this PR tested?