Skip to content

[SPARK-26351][mllib]Update doc and minor correction in the mllib evaluation metrics #23589

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.

Sign up for GitHub

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

Jump to bottom
Closed
shahidki31 wants to merge 6 commits into from
Closed

[SPARK-26351][mllib]Update doc and minor correction in the mllib evaluation metrics #23589

shahidki31 wants to merge 6 commits into from

Conversation

@shahidki31
Copy link
Contributor

@shahidki31 shahidki31 commented Jan 18, 2019 •
edited by srowen
Loading

What changes were proposed in this pull request?

Currently, there are some minor inconsistencies in doc compared to the code. In this PR, I am correcting those inconsistencies.

How was this patch tested?

NA

@SparkQA
Copy link

SparkQA commented Jan 18, 2019

Test build #101412 has finished for PR 23589 at commit 9eeeca8.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shahidki31
Copy link
Contributor Author

shahidki31 commented Jan 18, 2019

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Jan 18, 2019

Test build #101414 has finished for PR 23589 at commit c25b342.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@shahidki31
Copy link
Contributor Author

shahidki31 commented Jan 19, 2019

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Jan 19, 2019

Test build #101423 has finished for PR 23589 at commit a160561.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 19, 2019

Test build #101430 has finished for PR 23589 at commit b763bab.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen requested changes Jan 19, 2019
View reviewed changes
docs/mllib-evaluation-metrics.md Outdated
</td>
<td>
$p(k)=\frac{1}{M} \sum_{i=0}^{M-1} {\frac{1}{k} \sum_{j=0}^{\text{min}(\left|D\right|, k) - 1} rel_{D_i}(R_i(j))}$
$p(k)=\frac{1}{M} \sum_{i=0}^{M-1} {\frac{1}{k} \sum_{j=0}^{\text{min}(\left|R_i\right|, k) - 1} rel_{D_i}(R_i(j))}$
Copy link
Member

@srowen srowen Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we usually have k <= |D| and k <= |R_i|, I agree that this is technically what must have been meant. How about using Q here instead of |R_i|? it was already used in the following formula. You can change both, sure, either way, as long as they're consistent.

Copy link
Contributor Author

@shahidki31 shahidki31 Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the Q will vary with i, that is why I gave the notation |R_i|.

Copy link
Member

@srowen srowen Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe; it could be the same for all users, or not. The documentation above this suggests there are equal numbers of recommended and relevant docs for each user (Q and N) but at least, it will almost never be true that |D_i| is the same for all users. Q could well be a constant.

But the implementation doesn't assume that and it's not necessary to, so I might even just remove the references to Q and N, or label them "Q_i" and "N_i" if you really want to be complete.

Copy link
Contributor Author

@shahidki31 shahidki31 Jan 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. updated.

@shahidki31 shahidki31 force-pushed the docCorrection branch 2 times, most recently from 542d687 to 60bec8e Compare January 19, 2019 16:28
@shahidki31
Copy link
Contributor Author

shahidki31 commented Jan 19, 2019

Thank you @srowen for the review.

@SparkQA
Copy link

SparkQA commented Jan 19, 2019

Test build #101434 has finished for PR 23589 at commit 542d687.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jan 19, 2019

Test build #101435 has finished for PR 23589 at commit 2003599.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen reviewed Jan 19, 2019
View reviewed changes
docs/mllib-evaluation-metrics.md Outdated
</td>
<td>
$p(k)=\frac{1}{M} \sum_{i=0}^{M-1} {\frac{1}{k} \sum_{j=0}^{\text{min}(\left|D\right|, k) - 1} rel_{D_i}(R_i(j))}$
$p(k)=\frac{1}{M} \sum_{i=0}^{M-1} {\frac{1}{k} \sum_{j=0}^{\text{min}(\left|R_i\right|, k) - 1} rel_{D_i}(R_i(j))}$
Copy link
Member

@srowen srowen Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe; it could be the same for all users, or not. The documentation above this suggests there are equal numbers of recommended and relevant docs for each user (Q and N) but at least, it will almost never be true that |D_i| is the same for all users. Q could well be a constant.

But the implementation doesn't assume that and it's not necessary to, so I might even just remove the references to Q and N, or label them "Q_i" and "N_i" if you really want to be complete.

@SparkQA
Copy link

SparkQA commented Jan 20, 2019

Test build #101439 has finished for PR 23589 at commit d89fd0d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen pushed a commit that referenced this pull request Jan 21, 2019
@shahidki31 @srowen
[SPARK-26351][MLLIB] Update doc and minor correction in the mllib eva…
123adbd 
…luation metrics

## What changes were proposed in this pull request?
Currently, there are some minor inconsistencies in doc compared to the code. In this PR, I am correcting those inconsistencies.
1) Links related to the evaluation metrics in the docs are not working
2) Minor correction in the evaluation metrics formulas in docs.

## How was this patch tested?

NA

Closes #23589 from shahidki31/docCorrection.

Authored-by: Shahid <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit 9a30e23)
Signed-off-by: Sean Owen <[email protected]>
srowen pushed a commit that referenced this pull request Jan 21, 2019
@shahidki31 @srowen
[SPARK-26351][MLLIB] Update doc and minor correction in the mllib eva…
ae64e5b 
…luation metrics

## What changes were proposed in this pull request?
Currently, there are some minor inconsistencies in doc compared to the code. In this PR, I am correcting those inconsistencies.
1) Links related to the evaluation metrics in the docs are not working
2) Minor correction in the evaluation metrics formulas in docs.

## How was this patch tested?

NA

Closes #23589 from shahidki31/docCorrection.

Authored-by: Shahid <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit 9a30e23)
Signed-off-by: Sean Owen <[email protected]>
@srowen
Copy link
Member

srowen commented Jan 21, 2019

Merged to master/2.4/2.3

@srowen srowen closed this in 9a30e23 Jan 21, 2019
@shahidki31
Copy link
Contributor Author

shahidki31 commented Jan 21, 2019

Thank you @srowen

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@shahidki31
[SPARK-26351][MLLIB] Update doc and minor correction in the mllib eva…
682fbc5 
…luation metrics

## What changes were proposed in this pull request?
Currently, there are some minor inconsistencies in doc compared to the code. In this PR, I am correcting those inconsistencies.
1) Links related to the evaluation metrics in the docs are not working
2) Minor correction in the evaluation metrics formulas in docs.

## How was this patch tested?

NA

Closes apache#23589 from shahidki31/docCorrection.

Authored-by: Shahid <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 23, 2019
@shahidki31 @kai-chi
[SPARK-26351][MLLIB] Update doc and minor correction in the mllib eva…
0051561 
…luation metrics

## What changes were proposed in this pull request?
Currently, there are some minor inconsistencies in doc compared to the code. In this PR, I am correcting those inconsistencies.
1) Links related to the evaluation metrics in the docs are not working
2) Minor correction in the evaluation metrics formulas in docs.

## How was this patch tested?

NA

Closes apache#23589 from shahidki31/docCorrection.

Authored-by: Shahid <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit 9a30e23)
Signed-off-by: Sean Owen <[email protected]>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Aug 1, 2019
@shahidki31 @kai-chi
[SPARK-26351][MLLIB] Update doc and minor correction in the mllib eva…
2523137 
…luation metrics

## What changes were proposed in this pull request?
Currently, there are some minor inconsistencies in doc compared to the code. In this PR, I am correcting those inconsistencies.
1) Links related to the evaluation metrics in the docs are not working
2) Minor correction in the evaluation metrics formulas in docs.

## How was this patch tested?

NA

Closes apache#23589 from shahidki31/docCorrection.

Authored-by: Shahid <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit 9a30e23)
Signed-off-by: Sean Owen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srowen srowen srowen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shahidki31 @SparkQA @srowen