Skip to content

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 <[email protected]>

Closes #16675 from actuaryzhang/family.

(cherry picked from commit f067ace)
Signed-off-by: Yanbo Liang <[email protected]>
@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 <[email protected]>

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 <[email protected]>

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