Skip to content

Add mu and sigma to LogNormal and raise AttributeError for unknown names - #433

Merged
alejandroschuler merged 5 commits into
masterfrom
fix/lognormal-attributes
Nov 10, 2026
Merged

alejandroschuler merged 5 commits into
masterfrom
fix/lognormal-attributes

Conversation

@alejandroschuler

@alejandroschuler alejandroschuler commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #168.

Problem

For LogNormal, dist.params uses the names from scipy.stats.lognorm: params['s'] is σ and params['scale'] is exp(μ), where μ and σ are the mean and standard deviation of log(Y). The attributes dist.loc and dist.scale held μ and σ. So dist.scale and dist.params['scale'] had the same name but different values. Also, dist.s returned None without an error, because the __getattr__ of the distributions built on scipy.stats returned None for every name that the scipy object does not have.

Changes

  • LogNormal has new attributes mu and sigma. loc and scale are now properties. They still return μ and σ, but they give a FutureWarning that names the replacement. The LogNormal scores use mu and sigma, so fitting gives no warning. params does not change.
  • Distn has one __getattr__. It forwards a name to self.dist and raises AttributeError for all other names. It replaces the copies in Normal, LogNormal, Exponential, Gamma, HalfNormal, Laplace, Poisson, the three T classes and Weibull. It reads dist from __dict__, so pickle.loads and copy.deepcopy of a predicted distribution now work. Before, both raised RecursionError.
  • Docs: the user guide explains the LogNormal params keys and the new attributes. The developer guide no longer tells authors to write a __getattr__ that returns None.
  • tests/test_distn_attributes.py tests these changes. 24 of its 26 tests fail on master.

Why not the 2020 plan

The issue thread proposed a leading underscore on the attributes of all distributions. This PR does not do that:

  • For the other distributions, the attributes and params agree, and a lot of code reads .loc and .scale on Normal. Examples are NREL's routee-powertrain and this repo's figures/.
  • The SymPy factory from SymPy-powered distribution factory for NGBoost #403 names the attributes after the parameter symbols and passes them to user sample_fn and mean_fn. The example notebooks also read the attributes.

This PR also does not change LogNormal params. skpro's NGBoostRegressor and NGBoostSurvival compute μ as log(params['scale']) and σ as params['s']. A change in meaning would give their users wrong numbers without an error.

Compatibility

  • Pickled models: a model stores the distribution class and the internal parameters (μ, log σ), not distribution objects. Models pickled on master load on this branch with identical predictions and params, and they give no warning. I checked this for LogNormal regression, LogNormal survival, Exponential survival and Normal.
  • skpro 2.16 NGBoostRegressor(dist="LogNormal") gives the same means as ngboost on this branch.
  • hasattr(dist, name) is now False for a name that the distribution does not have. Before, it was always True.
  • This PR does not set a release for the removal of loc and scale. The warning says "a future release".

Tests

  • pytest tests --slow -o "filterwarnings=error::FutureWarning:ngboost" on 6282515: 190 passed, 1 skipped, 2 xfailed, 22 xpassed. No FutureWarning comes from ngboost code, so nothing in ngboost reads LogNormal.loc or LogNormal.scale.
    • The skip is the skops test in tests/test_pickling.py, because skops was not installed. With skops installed, that file gives 4 passed.
    • The 2 xfails are the T cases of test_dists_runs_on_examples_logscore. The 22 xpasses are the other cases of that test. They come from the existing xfail at tests/test_distns.py:91. Its condition is a function, which is always true.
  • Why -o and not -W: an earlier version of this description gave pytest tests -W error::FutureWarning:ngboost. That filter does not do this check. On the command line, pytest escapes the module name and matches it exactly. So it catches only warnings from the module ngboost, not from ngboost.distns.lognormal. In the ini option filterwarnings, the module is a regex that matches at the start of the name, so it also catches the submodules. To test the filter, I put one read of self.scale in the LogNormal LogScore metric in a scratch copy. With -W, test_survival_runs_on_examples passed with 500 warnings. With -o, it failed.
  • black, isort, flake8 and pylint, as in .pre-commit-config.yaml.

Overlap

#431 also edits docs/content/1-useage.ipynb and docs/content/5-dev.ipynb. This PR changes source lines in two cells of each notebook, so a conflict, if any, will be small.

🤖 Generated with Claude Code

alejandroschuler and others added 2 commits October 26, 2026 21:32
LogNormal.scale was sigma, the standard deviation of log(Y), but
params['scale'] is exp(mu), as in scipy.stats.lognorm, so users read the
wrong number (#168). LogNormal now has the attributes mu and sigma. The
attributes loc and scale still return mu and sigma, but they give a
FutureWarning. params does not change, so code that reads params['s'] and
params['scale'], such as the skpro adapters, keeps working.

The distributions built on scipy.stats returned None for any name they did
not have, so a typo such as dist.s failed silently. One __getattr__ in Distn
now forwards names to self.dist and raises AttributeError for the rest. It
reads dist from __dict__, so predicted distributions can now be pickled and
deep-copied. Before, both raised RecursionError.

Models saved with earlier versions load and predict the same, because a
model stores the distribution class and the internal parameters, not
distribution objects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pylint on tests flagged the two bare expressions as expression-not-assigned
(W0106), which failed the lint job. The test now checks that params has the
keys s and scale, and it imports the score classes at the top of the file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ryan-wolbeck ryan-wolbeck left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

will need to fix some merge conflicts with 431 but looks good

alejandroschuler and others added 3 commits November 9, 2026 12:05
Resolve the conflict in RELEASE_NOTES.md. Master rewrote the 0.6.0
notes into sections. Keep that version and add the two notes for
issue #168: the LogNormal mu and sigma attributes under New features,
and the AttributeError change under Bug fixes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#434 adds its bug-fix notes after the #409 line too, so the two PRs
conflicted in RELEASE_NOTES.md. With this note after the #422 line, they
merge cleanly in either order.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pre-commit splits the test files over parallel pylint runs, one per
CPU. duplicate-code only compares files in the same run, so its result
depends on the CPU count and on which files exist. With the new test
files of #433 and #434, the copy of the old-tree pickle helper in
test_helpers.py and test_pickling.py lands in one run on a 4-CPU
GitHub runner, and the hook fails. master already fails the same way
when pylint gets all test files at once.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alejandroschuler

Copy link
Copy Markdown
Collaborator Author

Thanks! The conflicts with #431 are fixed. f78949e merges master (with #431) into this branch, and the docs build, which runs the notebooks, passes.

I added two small commits so that this PR and #434 merge cleanly together:

@alejandroschuler
alejandroschuler merged commit e97a1bc into master Nov 10, 2026
7 checks passed
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.

Probelms with the output for regression with LogNormal

2 participants