Disable early stopping criteria #484
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You are correct that this part of the code is probably the reason why coefficients for at most 5 alphas are returned. However, this check is also part of the reference implementation in the R glmnet package: https://github.com/cran/glmnet/blob/ace3461223a8fe8a3e0c2f749d3d7e8e84e49ea8/src/glmnet3.f90#L4460 Hence, the code does have its purpose. The main question is where the two implementations deviate from each other. I would suggest to use an example where sksurv terminates early and compare the steps in glmnet and sksurv with a debugger. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
pytest passespytest (that were passing before) passesWhat does this implement/fix? Explain your changes
In
sksurv/linear_model/src/coxnet/coxnet.hwe find thefitmethod for theCoxnetSurvivalAnalysisclass. This fits a model for the desired regularization path. The current implementation includes an early stop condition which stops fiting a model for the given alphas based on a deviance ratio. However it is desirable sometimes to fit the whole path (using all the alphas) even if the performance does not change significanly across alphas (see #41 for a more detailed explanation of the behaviour). The obvios solution is to get rid of the early stop condition (which is what I have done here by commentitg it out), however I think the better solution would be to include a boolean flag in the class definition that specifies if the user wishes or not to use this early stopping functionality. Sadly I do not have the time to implement such a solution. Please feel free to use this PR as a starting point.