You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "zhipeng93 (via GitHub)" <gi...@apache.org> on 2023/06/09 02:17:11 UTC
[GitHub] [flink-ml] zhipeng93 opened a new pull request, #242: [FLINK-32293] Support sparse vector with long index
zhipeng93 opened a new pull request, #242:
URL: https://github.com/apache/flink-ml/pull/242
## What is the purpose of the change
Add support for sparse vector with long as index and double as value.
## Brief change log
- Renamed `SparseVector` as `SparseIntDoubleVector`, `DenseVector` as `DenseIntDoubleVector` and added `SparseLongDoubleVector`
- Added test for SparseLongDouble vector
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes)
## Documentation
- Does this pull request introduce a new feature? (yes)
- If yes, how is the feature documented? (JavaDocs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] lindong28 commented on a diff in pull request #242: [FLINK-32293] Support sparse vector with long index
Posted by "lindong28 (via GitHub)" <gi...@apache.org>.
lindong28 commented on code in PR #242:
URL: https://github.com/apache/flink-ml/pull/242#discussion_r1225027030
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
Review Comment:
Given that we only need to store values as double would it be simpler to remove `V extends Number`?
We can extend the interface to support using float as the value type. But given that Spark ML does not provide this optimization, it is not clear whether/when we will ever need it.
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
/** Gets the size of the vector. */
- int size();
+ K size();
/** Gets the value of the ith element. */
- double get(int i);
+ V get(K i);
/** Sets the value of the ith element. */
- void set(int i, double value);
+ void set(K i, V value);
Review Comment:
Would it be simpler to use `void set(long i, double value)`?
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
/** Gets the size of the vector. */
- int size();
+ K size();
Review Comment:
Would it be simpler to use `long size()`?
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
/** Gets the size of the vector. */
- int size();
+ K size();
/** Gets the value of the ith element. */
- double get(int i);
+ V get(K i);
/** Sets the value of the ith element. */
- void set(int i, double value);
+ void set(K i, V value);
- /** Converts the instance to a double array. */
- double[] toArray();
+ /** Converts the instance to a primitive array. */
+ Object toArray();
/** Converts the instance to a dense vector. */
- DenseVector toDense();
+ Vector<K, V> toDense();
/** Converts the instance to a sparse vector. */
- SparseVector toSparse();
+ Vector<K, V> toSparse();
Review Comment:
The intention of defining this method in an interface is to have the caller use the return value via public APIs of `Vector<K, V>`.
However, it appears that caller of this method uses return values via public fields/APIs that do not belong to `Vector<K, V>`. For example, `MinHashLSHModelData#hashFunction()` accesses the field `SparseIntDoubleVector#indices` via `vec.toSparse().indices`. The code will break if we change the concrete class from `SparseIntDoubleVector` to `SparseLongDoubleVector`.
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
/** Gets the size of the vector. */
- int size();
+ K size();
/** Gets the value of the ith element. */
- double get(int i);
+ V get(K i);
Review Comment:
Would it be simpler to just use `double get(long i)`?
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
/** Gets the size of the vector. */
- int size();
+ K size();
/** Gets the value of the ith element. */
- double get(int i);
+ V get(K i);
/** Sets the value of the ith element. */
- void set(int i, double value);
+ void set(K i, V value);
- /** Converts the instance to a double array. */
- double[] toArray();
+ /** Converts the instance to a primitive array. */
+ Object toArray();
Review Comment:
The implementation of this API always return `double[]`. Would it be simpler to still use `double[] toArray()`?
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/SparseLongDoubleVector.java:
##########
@@ -0,0 +1,174 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.flink.ml.linalg;
+
+import org.apache.flink.annotation.PublicEvolving;
+import org.apache.flink.api.common.typeinfo.TypeInfo;
+import org.apache.flink.ml.linalg.typeinfo.SparseLongDoubleVectorTypeInfoFactory;
+import org.apache.flink.util.Preconditions;
+
+import java.util.Arrays;
+
+/**
+ * A sparse vector with long as keys and double as values.
+ *
+ * <p>TODO: Add processing logic for {@link SparseLongDoubleVector} for existing algorithms.
+ */
+@TypeInfo(SparseLongDoubleVectorTypeInfoFactory.class)
+@PublicEvolving
+public class SparseLongDoubleVector implements LongDoubleVector {
+
+ public final long n;
+ public long[] indices;
Review Comment:
Would it be useful to make these fields private and expose them via methods (e.g. `getIndices()` or `getIndicesAsLongArray()`) so that we can use the same caller code for different implementations of SparseVector?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #242: [FLINK-32293] Support sparse vector with long index
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #242:
URL: https://github.com/apache/flink-ml/pull/242#discussion_r1226060309
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
Review Comment:
I think keeping `V extends Number` would make this more scalable. Though Spark does not support float32 as value, but storing data in other data types (e.g., float32) does save the space.
I searched the data types used in some machine learning libraries and math lib.
- Breeze supports int-key and double/float/int/long values [1]. Breeze is used by Spark ML, but Spark ML restricts the value type as double.
- Sklearn uses numpy array as input, the data types of numpy array could be float16, single, double etc. [2]
- TensorFlow/PyTorch supports different data types of tensors [3]
[1] https://github.com/scalanlp/breeze/blob/master/math/src/main/scala/breeze/linalg/DenseVector.scala
[2] https://numpy.org/doc/stable/user/basics.types.html
[3] https://www.tensorflow.org/guide/tensor#more_on_dtypes
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
Review Comment:
I think keeping `V extends Number` would make this interface more scalable. Though Spark does not support float32 as value, but storing data in other data types (e.g., float32) does save the space.
I searched the data types used in some machine learning libraries and math lib.
- Breeze supports int-key and double/float/int/long values [1]. Breeze is used by Spark ML, but Spark ML restricts the value type as double.
- Sklearn uses numpy array as input, the data types of numpy array could be float16, single, double etc. [2]
- TensorFlow/PyTorch supports different data types of tensors [3]
[1] https://github.com/scalanlp/breeze/blob/master/math/src/main/scala/breeze/linalg/DenseVector.scala
[2] https://numpy.org/doc/stable/user/basics.types.html
[3] https://www.tensorflow.org/guide/tensor#more_on_dtypes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on a diff in pull request #242: [FLINK-32293] Support sparse vector with long index
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on code in PR #242:
URL: https://github.com/apache/flink-ml/pull/242#discussion_r1226064974
##########
flink-ml-servable-core/src/main/java/org/apache/flink/ml/linalg/Vector.java:
##########
@@ -24,29 +24,29 @@
import java.io.Serializable;
-/** A vector of double values. */
+/** A vector representation of numbers. */
@TypeInfo(VectorTypeInfoFactory.class)
@PublicEvolving
-public interface Vector extends Serializable {
+public interface Vector<K extends Number, V extends Number> extends Serializable {
/** Gets the size of the vector. */
- int size();
+ K size();
/** Gets the value of the ith element. */
- double get(int i);
+ V get(K i);
/** Sets the value of the ith element. */
- void set(int i, double value);
+ void set(K i, V value);
- /** Converts the instance to a double array. */
- double[] toArray();
+ /** Converts the instance to a primitive array. */
+ Object toArray();
/** Converts the instance to a dense vector. */
- DenseVector toDense();
+ Vector<K, V> toDense();
/** Converts the instance to a sparse vector. */
- SparseVector toSparse();
+ Vector<K, V> toSparse();
Review Comment:
I have overrode the return type in `IntDoubleVector` and `LongDoubleVector`, since for different instances `toSparse` should return different types of vectors.
We can talk about this offline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 commented on pull request #242: [FLINK-32293] Support sparse vector with long index
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 commented on PR #242:
URL: https://github.com/apache/flink-ml/pull/242#issuecomment-1584281026
@lindong28 @HuangXingBo Can you help to review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-ml] zhipeng93 closed pull request #242: [FLINK-32293] Support sparse vector with long index
Posted by "zhipeng93 (via GitHub)" <gi...@apache.org>.
zhipeng93 closed pull request #242: [FLINK-32293] Support sparse vector with long index
URL: https://github.com/apache/flink-ml/pull/242
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org