Skip to content

[SPARK-19155][ML] Make family case insensitive in GLM #16675

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

Closed
wants to merge 3 commits into from

Conversation

actuaryzhang
Copy link
Contributor

@actuaryzhang actuaryzhang commented Jan 23, 2017

What changes were proposed in this pull request?

This is a supplement to PR #16516 which did not make the value from getFamily case insensitive. Current tests of poisson/binomial glm with weight fail when specifying 'Poisson' or 'Binomial', because the calculation of dispersion and pValue checks the value of family retrieved from getFamily

model.getFamily == Binomial.name || model.getFamily == Poisson.name

How was this patch tested?

Update existing tests for 'Poisson' and 'Binomial'.

@yanboliang @felixcheung @imatiach-msft

@actuaryzhang actuaryzhang changed the title [SPARK-19155][ML] make getFamily case insensitive [SPARK-19155][ML] Make family case insensitive in GLM Jan 23, 2017
@actuaryzhang
Copy link
Contributor Author

I would prefer that getFamily returns lower case values directly, because using getFamily.toLowerCase can get very cumbersome and I use this a lot in another PR #16344. If we want to keep getFamily to retrieve the raw value of family, then I can create a private method getFamilyLowerCase. Please advise.

@yanboliang
Copy link
Contributor

@actuaryzhang I think the change is not appropriate, the function getFamily should return the raw value that users specified, this is the cause that I didn't change them in #16516 . Thanks.

@actuaryzhang
Copy link
Contributor Author

@yanboliang Thanks for the quick response. How about the new commit, where I just change the value from getFamily to lower case when necessary, i.e., in the calculation of p-value and dispersion?

@yanboliang
Copy link
Contributor

Looks good, I'll merge if it passes test. Thanks.

@SparkQA
Copy link

SparkQA commented Jan 23, 2017

Test build #71819 has finished for PR 16675 at commit c2b4132.

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

@SparkQA
Copy link

SparkQA commented Jan 23, 2017

Test build #71820 has finished for PR 16675 at commit 97b0a1c.

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

@actuaryzhang
Copy link
Contributor Author

@yanboliang Thanks. Seems to have passed tests.

asfgit pushed a commit that referenced this pull request Jan 23, 2017
## What changes were proposed in this pull request?
This is a supplement to PR #16516 which did not make the value from `getFamily` case insensitive. Current tests of poisson/binomial glm with weight fail when specifying 'Poisson' or 'Binomial', because the calculation of `dispersion` and `pValue` checks the value of family retrieved from `getFamily`
```
model.getFamily == Binomial.name || model.getFamily == Poisson.name
```

## How was this patch tested?
Update existing tests for 'Poisson' and 'Binomial'.

yanboliang felixcheung imatiach-msft

Author: actuaryzhang <actuaryzhang10@gmail.com>

Closes #16675 from actuaryzhang/family.

(cherry picked from commit f067ace)
Signed-off-by: Yanbo Liang <ybliang8@gmail.com>
@yanboliang
Copy link
Contributor

LGTM, merged into master and branch-2.1. Thanks.

@asfgit asfgit closed this in f067ace Jan 23, 2017
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?
This is a supplement to PR apache#16516 which did not make the value from `getFamily` case insensitive. Current tests of poisson/binomial glm with weight fail when specifying 'Poisson' or 'Binomial', because the calculation of `dispersion` and `pValue` checks the value of family retrieved from `getFamily`
```
model.getFamily == Binomial.name || model.getFamily == Poisson.name
```

## How was this patch tested?
Update existing tests for 'Poisson' and 'Binomial'.

yanboliang felixcheung imatiach-msft

Author: actuaryzhang <actuaryzhang10@gmail.com>

Closes apache#16675 from actuaryzhang/family.
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
## What changes were proposed in this pull request?
This is a supplement to PR apache#16516 which did not make the value from `getFamily` case insensitive. Current tests of poisson/binomial glm with weight fail when specifying 'Poisson' or 'Binomial', because the calculation of `dispersion` and `pValue` checks the value of family retrieved from `getFamily`
```
model.getFamily == Binomial.name || model.getFamily == Poisson.name
```

## How was this patch tested?
Update existing tests for 'Poisson' and 'Binomial'.

yanboliang felixcheung imatiach-msft

Author: actuaryzhang <actuaryzhang10@gmail.com>

Closes apache#16675 from actuaryzhang/family.
@actuaryzhang actuaryzhang deleted the family branch May 9, 2017 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants