-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-10834: Add NamedVector into ML module. #5881
Conversation
*/ | ||
public static NamedVector of(Map<String, Double> values) { | ||
SparseVector vector = new SparseVector(values.size(), StorageConstants.RANDOM_ACCESS_MODE); | ||
for (int i = 0; i < values.size(); i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for Double.NaN setting? You don't use these values below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
* Delegating named vector that delegates all operations to underlying vector and adds implementation of | ||
* {@link NamedValue} functionality using embedded map that maps string index on real integer index. | ||
*/ | ||
public class DelegatingNamedVector extends DelegatingVector implements NamedVector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that vector operations hide real type of result - sum of NamedVectors will be Vector. Moreover, there is no checks of keys equality for different NamedVectors. Maybe we need generics?))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense. I've slightly changed approach and made SimpleNamedVector
that implements AbstractVector
, at the same time AbstractVector
is more generic. Please, have a look.
No description provided.