You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/09 07:54:33 UTC
[GitHub] [spark] ahmed-mahran opened a new pull request, #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
ahmed-mahran opened a new pull request, #38996:
URL: https://github.com/apache/spark/pull/38996
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
A follow-up on https://github.com/apache/spark/pull/38966 to update relevant documentation and remove redundant sort key.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
For isotonic regression, another method for breaking ties of repeated features was introduced in https://github.com/apache/spark/pull/38966. This will aggregate points having the same feature value by computing the weighted average of the labels.
- This only requires points to be sorted by features instead of features and labels. So, we should remove label as a secondary sorting key.
- Isotonic regression documentation needs to be updated to reflect the new behavior.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Isotonic regression documentation update. The documentation described the behavior of the algorithm when there are points in the input with repeated features. Since this behavior has changed, documentation needs to describe the new behavior.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Existing tests passed. No need to add new tests since existing tests are already comprehensive.
@srowen
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #38996:
URL: https://github.com/apache/spark/pull/38996#discussion_r1044938891
##########
docs/mllib-isotonic-regression.md:
##########
@@ -43,7 +43,17 @@ best fitting the original data points.
which uses an approach to
[parallelizing isotonic regression](https://doi.org/10.1007/978-3-642-99789-1_10).
The training input is an RDD of tuples of three double values that represent
-label, feature and weight in this order. Additionally, IsotonicRegression algorithm has one
+label, feature and weight in this order. In case there are multiple tuples with
+the same feature then these tuples are aggregated into a single tuple as follows:
+
+* Aggregated label is the weighted average of all labels.
+* Aggregated feature is the weighted average of all equal features. It is possible
Review Comment:
(Sorry if this double-posts)
Yeah I was confused about this too. If feature values are pooled when exactly equal, why average them? if anything that introduces tiny errors
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ahmed-mahran commented on a diff in pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
ahmed-mahran commented on code in PR #38996:
URL: https://github.com/apache/spark/pull/38996#discussion_r1045202051
##########
docs/mllib-isotonic-regression.md:
##########
@@ -43,7 +43,17 @@ best fitting the original data points.
which uses an approach to
[parallelizing isotonic regression](https://doi.org/10.1007/978-3-642-99789-1_10).
The training input is an RDD of tuples of three double values that represent
-label, feature and weight in this order. Additionally, IsotonicRegression algorithm has one
+label, feature and weight in this order. In case there are multiple tuples with
+the same feature then these tuples are aggregated into a single tuple as follows:
+
+* Aggregated label is the weighted average of all labels.
+* Aggregated feature is the weighted average of all equal features. It is possible
Review Comment:
Now it is exact equality likewise other parts of the implementation that don't handle approximation errors, and it should be the responsibility of the user to handle such cases before fitting the model.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ahmed-mahran commented on pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
ahmed-mahran commented on PR #38996:
URL: https://github.com/apache/spark/pull/38996#issuecomment-1345654534
Just modified tests I added in my previous PR. In one, just used same values as in the ticket. In the other, just modified to test exact equality instead of approximate.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ahmed-mahran commented on a diff in pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
ahmed-mahran commented on code in PR #38996:
URL: https://github.com/apache/spark/pull/38996#discussion_r1045109966
##########
docs/mllib-isotonic-regression.md:
##########
@@ -43,7 +43,17 @@ best fitting the original data points.
which uses an approach to
[parallelizing isotonic regression](https://doi.org/10.1007/978-3-642-99789-1_10).
The training input is an RDD of tuples of three double values that represent
-label, feature and weight in this order. Additionally, IsotonicRegression algorithm has one
+label, feature and weight in this order. In case there are multiple tuples with
+the same feature then these tuples are aggregated into a single tuple as follows:
+
+* Aggregated label is the weighted average of all labels.
+* Aggregated feature is the weighted average of all equal features. It is possible
Review Comment:
It's just the default thought when it comes to comparing doubles. On the other hand though, other parts of the implementation don't care: in partitioning by features and in combining labels into isotonic buckets. So, why not do the same here as well. So, exact equality sounds fit here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #38996:
URL: https://github.com/apache/spark/pull/38996#issuecomment-1345655384
SGTM. Merged to master
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] ahmed-mahran commented on a diff in pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
ahmed-mahran commented on code in PR #38996:
URL: https://github.com/apache/spark/pull/38996#discussion_r1045077710
##########
docs/mllib-isotonic-regression.md:
##########
@@ -43,7 +43,17 @@ best fitting the original data points.
which uses an approach to
[parallelizing isotonic regression](https://doi.org/10.1007/978-3-642-99789-1_10).
The training input is an RDD of tuples of three double values that represent
-label, feature and weight in this order. Additionally, IsotonicRegression algorithm has one
+label, feature and weight in this order. In case there are multiple tuples with
+the same feature then these tuples are aggregated into a single tuple as follows:
+
+* Aggregated label is the weighted average of all labels.
+* Aggregated feature is the weighted average of all equal features. It is possible
Review Comment:
Equality is not exact, it is defined by [org.apache.commons.math3.util.Precision.equals(double x, double y)](https://commons.apache.org/proper/commons-math/javadocs/api-3.6.1/org/apache/commons/math3/util/Precision.html#equals(double,%20double)) which is true whenever x and y are exactly equal or are adjacent doubles (i.e. no other representable doubles exist in between). So, there has to be pooling of features too. The weighted average here just guarantees fairness. scikit-learn uses first encountered (i.e. minimum) feature.
Definition of equality in scikit-learn is different too:
```python
eps = np.finfo(np.floating).resolution # 1e-15
if x - current_x >= eps:
# different
else:
# equal
```
We can use minimum for consistency, avoid accumulative errors, and for better performance too. Do you agree?
What do you suggest regarding definition of equality?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
URL: https://github.com/apache/spark/pull/38996
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38996:
URL: https://github.com/apache/spark/pull/38996#issuecomment-1345256275
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #38996: [SPARK-41008][MLLIB] Follow-up isotonic regression features deduplica…
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #38996:
URL: https://github.com/apache/spark/pull/38996#discussion_r1045090652
##########
docs/mllib-isotonic-regression.md:
##########
@@ -43,7 +43,17 @@ best fitting the original data points.
which uses an approach to
[parallelizing isotonic regression](https://doi.org/10.1007/978-3-642-99789-1_10).
The training input is an RDD of tuples of three double values that represent
-label, feature and weight in this order. Additionally, IsotonicRegression algorithm has one
+label, feature and weight in this order. In case there are multiple tuples with
+the same feature then these tuples are aggregated into a single tuple as follows:
+
+* Aggregated label is the weighted average of all labels.
+* Aggregated feature is the weighted average of all equal features. It is possible
Review Comment:
Oh OK I see it. But why bother, just deal with duplicates that are exactly equal and skip this? is it to match scikit? They are already distinct and ordered if they're different even by a small amount
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org