Repository navigation
Return df in T.params and fix the Cauchy starting fit - #450
Open
alejandroschuler wants to merge 2 commits into
Open
alejandroschuler wants to merge 2 commits into
alejandroschuler wants to merge 2 commits into
Conversation
T.params now returns df, the fitted degrees of freedom, next to loc and scale. So scipy.stats.t(**dist.params) gives the predicted distribution. TFixedDf.fit and TFixedDfFixedVar.fit read fixed_df from the class they are called on. Before, they read it from TFixedDf and TFixedDfFixedVar, so Cauchy and CauchyFixedVar (and user subclasses with their own fixed_df) started from the df = 3 fit. TFixedDfFixedVar.fit also holds the scale at 1 now, as the model does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tests/test_helpers.py had its own copy of the two old-pickle helpers in tests/test_pickling.py. pylint reports this as duplicate-code (R0801) when pre-commit puts both files in the same batch, and the new test file changed the batches so that it did. test_helpers.py now imports the helpers from test_pickling.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or 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
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.
Summary
Two small fixes in the Student t family (
ngboost/distns/t.py).T.paramsnow includesdf.Tfits the degrees of freedom, butparamsreturned onlylocandscale. Now it returnsloc,scaleanddf, soscipy.stats.t(**dist.params)gives the predicted distribution. Before, that call failed becausedfwas missing. The fixed-df classes (TFixedDf,TFixedDfFixedVar,Cauchy,CauchyFixedVar) keeplocandscale, because their df is a constant and theCauchykeys matchscipy.stats.cauchy.Users of
T.paramsthat I checked:tests/test_basic.pyreadspred_dist(...).params. It loops over the keys, so the new key is safe there.skpro/regression/adapters/ngboost/_ngboost_proba.py) readsparams["loc"]andparams["scale"]by key, so it does not break. It builds itsTDistributionwithoutdf, so it gets skpro's defaultdf=1. WithdfinT.params, skpro can pass the fitted df through.T.paramsin a DataFrame, or that passes it to a constructor with onlylocandscale, gets one more key. The release notes say so.The starting fit of
CauchyandCauchyFixedVarused df = 3.CauchysubclassesTFixedDfwithfixed_df = 1.0, butTFixedDf.fitreadTFixedDf.fixed_df, so the marginal fit that setsinit_paramsused df = 3. On Cauchy data with scale 0.5,Cauchy.fitgave a scale of 0.89.CauchyFixedVarhad the same problem throughTFixedDfFixedVar.fit. User subclasses with their ownfixed_dfhad it too (some on GitHub subclassTFixedDfwithfixed_df = 5).TFixedDf.fitandTFixedDfFixedVar.fitare now classmethods that readcls.fixed_df.NGBoostcallsself.Manifold.fit(Y)on a class that subclasses the distribution, andSurvivalDistnClasscallsDist.fit(Y["Time"]). Both work with a classmethod. I also checked that anNGBRegressorwithCauchystarts at the Cauchy MLE and still pickles.TFixedDfFixedVar.fitnow also passesfscale=1.0. The model holds the scale at 1, but the starting fit estimatedlocwith a free scale, so the startinglocwas not the MLE under the model.T.fitis not changed here. #443 rewrites it.Tests
New file
tests/test_t.py. All of its tests fail on master except theTFixedDfcontrol case:test_t_params_include_df:T.paramshasdf, andscipy.stats.t(**params)gives the same log density as the ngboost distribution.test_fixed_df_fit_uses_class_df(TFixedDf,Cauchy, a subclass withfixed_df = 5):fitequalsscipy.stats.t.fitwith that df.test_cauchy_fit_is_cauchy_mle:Cauchy.fiton Cauchy data equalsscipy.stats.cauchy.fit.test_fixed_df_fixed_var_fit_uses_class_df_and_unit_scale(TFixedDfFixedVar,CauchyFixedVar, a subclass withfixed_df = 5):fitequalsscipy.stats.t.fitwith that df and scale 1.The tests are in a new file because #442, #443 and #446 all append to
tests/test_distns.py.git merge-treemerges this branch with each open PR (#442, #443, #445 to #449) with no conflict.pytest testspasses. black, isort, flake8 and pylint (with the flags in.pre-commit-config.yaml) pass.The second commit fixes a lint failure that the new test file exposed.
tests/test_helpers.pyhad its own copy of the two old-pickle helpers intests/test_pickling.py. pylint reports that as duplicate-code (R0801) only when pre-commit puts both files in the same batch. A new file intests/changes the batches, and here it put them together.test_helpers.pynow imports the helpers fromtest_pickling.py, sopylint testspasses on all test files at once. I did not changetest_pickling.py, because #445 and #446 edit it.Docs
This PR does not change
docs/content/1-useage.ipynb. The distribution tables and their notes come from #447, which is not on master yet. After both PRs merge, #447's text needs two edits:Trow fromloc,scaletoloc,scale,df.Talso fits the degrees of freedom. They are not inparams, but thedfattribute of a predicted distribution gives them."🤖 Generated with Claude Code