Skip to content
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

ENH: Surface more errors from the glmnet solver #29

Merged
merged 3 commits into from
Dec 13, 2017

Conversation

wlattner
Copy link
Contributor

The glmnet solver uses integer codes to communicate errors and warnings. The error code returned by then solver is saved to the attribute jerr after fitting a model. Negative values denote warnings such as convergence issues, positive values denote fatal conditions such as memory allocation problems, and a value of zero is used when the solver runs successfully without error.

Initially we translated the convergence warnings into more complete messages from the numeric code. For all other errors, we just returned something opaque like "glmnet error no. 123."

In this PR, we add some additional messages and raise the relevant type of warning or exception.

The glmnet solver uses integer codes to communicate errors and warnings.
The error code returned by then solver is saved to the attribute `jerr`
after fitting a model. Negative values denote warnings such as
convergence issues, positive values denote fatal conditions such as
memory allocation problems, and a value of zero is used when the solver
runs successfully without error.

Initially we translated the convergence warnings into more complete
messages from the numeric code. For all other errors, we just returned
something opaque like "glmnet error no. 123."

In this PR, we add some additional messages and raise the relevant type
of warning or exception.
glmnet/errors.py Outdated
_convergence_errors(jerr, n_lambda)


def _fatal_errors(jerr, n_lambda):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to pass n_lambda here? Looks like the function doesn't refer to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's only there to make this function consistent with _convergence_errors. Although it looks like it may be necessary to decode the additional errors that could be returned by lognet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... looks like n_lambda isn't needed in either case. The convergence code is in [-k, -1].

glmnet/errors.py Outdated

def _check_glmnet_error_flag(jerr, n_lambda):
"""Check the error flag. Issue warning on convergence errors (jerr < 0)
and exception on anything else."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Your intro paragraph that describes the interpretation of jerr is good -- maybe stash it here in the docstring for a quick reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@xdavio
Copy link
Contributor

xdavio commented Dec 12, 2017

This probably isn't an issue, but I'm flagging it here for completeness: elnet errors and lognet errors are not exactly the same. But I think the general elnet errors you cover in this PR are the same for all the methods and are sufficient

@wlattner
Copy link
Contributor Author

This probably isn't an issue, but I'm flagging it here for completeness: elnet errors and lognet errors are not exactly the same.

It's only a few additional messages. I'll update the PR to include these.

@beckermr
Copy link

Hi Bill! (disappear)

Copy link
Contributor

@xdavio xdavio left a comment

Choose a reason for hiding this comment

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

LGTM

@xdavio xdavio merged commit b58df42 into civisanalytics:master Dec 13, 2017
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.

None yet

3 participants