IGNITE-11647: ML Vectors should work with all Serializable objects besides double #6378
Conversation
modules/ml/src/main/java/org/apache/ignite/ml/math/primitives/vector/AbstractVector.java
Show resolved
Hide resolved
modules/ml/src/main/java/org/apache/ignite/ml/math/primitives/vector/AbstractVector.java
Outdated
Show resolved
Hide resolved
modules/ml/src/main/java/org/apache/ignite/ml/math/primitives/vector/AbstractVector.java
Show resolved
Hide resolved
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public <T extends Serializable> T getRawX(int idx) { |
There was a problem hiding this comment.
Why somewhere you use checkIndex(idx), but somewhere not?
There was a problem hiding this comment.
Please read javadoc for get*X methods:
"Gets the value at specified index without checking for index boundaries"
modules/ml/src/main/java/org/apache/ignite/ml/math/primitives/vector/AbstractVector.java
Outdated
Show resolved
Hide resolved
|
|
||
| Serializable v = rawData[i]; | ||
| if (v == null) | ||
| return 0.0; |
There was a problem hiding this comment.
We use Double.NaN for missing values. Would it be better to use it here?
There was a problem hiding this comment.
No, it wasn't documented but old API returned 0.0 and I don't want to change this behavior.
There was a problem hiding this comment.
hmmm, support for Double.NaN but not in this ticket, could you make TODO + ticket for this, @avplatonov ?
There was a problem hiding this comment.
Yes, sure. But I think we should provide a way to use other user defined values for default values in Vector and use Double.NaN as default way in our code.
...ml/src/main/java/org/apache/ignite/ml/math/primitives/vector/storage/DenseVectorStorage.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public <T extends Serializable> void setRaw(int i, T v) { | ||
| swap(false); |
There was a problem hiding this comment.
Would it be better to keep two keep both arrays "active" and bitset that tells for each index which array contains actual value?
There was a problem hiding this comment.
No, I don't want to save additional states in vector. From my point of view, vector can be numeric and if we try to save some Serializable object - vector change own state to Serializabled representation. Only "double[] data()" can cast vector to numeric representation. These casts won't be so frequently.
...l/src/main/java/org/apache/ignite/ml/math/primitives/vector/storage/SparseVectorStorage.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/apache/ignite/ml/math/primitives/vector/storage/DenseVectorStorageTest.java
Outdated
Show resolved
Hide resolved
|
@dmitrievanthony thanks for review |
| * Tries to cast internal representation of data to array of Serializable objects if need. | ||
| */ | ||
| private void toGenericArray() { | ||
| A.ensure(data == null || rawData == null, "data == null || rawData == null"); |
There was a problem hiding this comment.
This is a standard way for assertions in Ignite.Core
|
|
||
| Serializable v = rawData[i]; | ||
| if (v == null) | ||
| return 0.0; |
There was a problem hiding this comment.
hmmm, support for Double.NaN but not in this ticket, could you make TODO + ticket for this, @avplatonov ?
|
The ticket is resolved, closing PR. Please close PRs for your resolved tickets and other non-needed PRs. Alternatively, ask a committer
These methods will automatically close PR. Feel free to reopen this PR if it is actual. |
No description provided.