[DOC] add explicit formulae to _formula_docs for Logistic and Weibull distributions - #1124
Merged
Conversation
All seven keys: pdf, log_pdf, cdf, ppf, mean, var, energy. Core formulae from Johnson, Kotz & Balakrishnan, Continuous Univariate Distributions, 2nd ed., Vol. 2 Ch. 23. Energy 2s is already stated and derived in this file’s own _energy_self docstring, and matches the Analytic column for Logistic(0,1) in energy_formulae.md. Note scale is the true scale s, not a rate: sd is s*pi/sqrt(3).
All seven keys: pdf, log_pdf, cdf, ppf, mean, var, energy. Core formulae from Johnson, Kotz & Balakrishnan, Continuous Univariate Distributions, 2nd ed., Vol. 1 Ch. 21. Energy derived from the fact that min(X, Y) of two iid Weibull(lambda, k) is Weibull(lambda * 2**(-1/k), k), so E|X-Y| = 2*(E[X] - E[min]); this reproduces the Analytic value 0.519140 for Weibull(1,2) already tabulated in energy_formulae.md. Note scale is the multiplicative scale lambda, not a rate, matching scipy.stats.weibull_min(c=k, scale=lambda). Support is x >= 0, as the _pdf and _cdf both multiply by (x >= 0).
Weibull was the UNHOOKED_CLASSES control group, so adding _formula_docs to it makes test_unhooked_clean_fallback fail on the ".. math::" assertion. Moves Weibull into HOOKED_CLASSES together with Logistic and LogNormal, and promotes Gamma to be the new control. Gamma has no _formula_docs and is not claimed on sktime#1120 at the time of writing.
lognormal.py is the only file of the three with CRLF line endings, and editing it through the GitHub web editor normalises them to LF, which turns a +60 line change into a 265-line whitespace diff. Rather than mix a line-ending change into a docs PR, LogNormal is left for a follow-up done as a local commit. This PR is therefore Logistic and Weibull only, and the test file is adjusted to match.
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.
Reference Issues/PRs
Towards #1120.
Does not overlap with #1123 (Uniform, @Vidit-lab) or #968 (Pareto). Claimed on the issue before starting.
What does this implement/fix? Explain your changes.
Populates
_formula_docsfor two distributions that previously had none:Logistic(skpro/distributions/logistic.py)Weibull(skpro/distributions/weibull.py)Each dict covers all seven applicable keys:
pdf,log_pdf,cdf,ppf,mean,var,energy, following theNormalexample and the extension template.I picked these two because they are native
BaseDistributionimplementations with elementary closed-formppfand exactmean/var, so every key is fillable without guessing. I looked at Poisson, Binomial and Beta first and skipped them: they are_ScipyAdapterwrappers whoseppfhas no closed form.This PR also has to touch
skpro/distributions/tests/test_docstring_injection.py. That test usedWeibullas theUNHOOKED_CLASSEScontrol group, so adding formulae toWeibullmadetest_unhooked_clean_fallbackfail on its".. math::" not in docassertion.Weibullmoves intoHOOKED_CLASSESalongsideLogistic, andGammabecomes the new control - it has no_formula_docsand is unclaimed on #1120 at the time of writing. Happy to use a different control if you would prefer one less likely to be claimed.References for the formulae
No AI-generated formulae.
Core
pdf/log_pdf/cdf/ppf/mean/varare standard results from Johnson, Kotz & Balakrishnan, Continuous Univariate Distributions, 2nd ed. - Vol. 2 Ch. 23 (Logistic) and Vol. 1 Ch. 21 (Weibull); cross-checked against the NIST/SEMATECH e-Handbook 1.3.6.6.The two energy formulae, stated explicitly since they need more than a citation:
E|X-Y| = 2s- already stated and derived in this repo, in the existingLogistic._energy_selfdocstring (logistic.pyL223-233).E|X-Y| = 2*lambda*Gamma(1 + 1/k)*(1 - 2**(-1/k))- derived from the fact that for iidX, Y ~ Weibull(lambda, k),min(X, Y) ~ Weibull(lambda * 2**(-1/k), k), soE|X-Y| = 2*(E[X] - E[min]).Both reproduce the Analytic column already tabulated in this repo's own
skpro/distributions/energy_formulae.md-Logistic(0,1) = 2.000000andWeibull(1,2) = 0.519140- which I treat as independent confirmation rather than my own arithmetic checking itself.Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
Gammaas the replacement control group intest_docstring_injection.py.energykey should be included at all, or left out until there is a dedicated review of energy formulae. Removing it is a clean deletion of one key per distribution; the other six are independent.Logistic.scaleis the true scales(so the sd iss*pi/sqrt(3)), andWeibull.scaleis the multiplicative scalelambda, matchingscipy.stats.weibull_min(c=k, scale=lambda). Neither is a rate. Weibull's documented support isx >= 0because_pdfand_cdfboth multiply by(x >= 0).Did you add any tests for the change?
No new tests. The existing
test_docstring_injection.pyalready covers exactly this change:test_hooked_math_injectionnow asserts thatLogisticandWeibullrender.. math::, andtest_placeholder_removal_universalasserts no{formula_doc}leak. 95 tests pass locally.Separately from the test suite, I verified every formula numerically against each class's own
_pdf/_log_pdf/_cdf/_ppf/_mean/_varover several parameter settings (max deviation ~1e-15), and the energy closed forms against quadrature of2 * integral F(t)(1-F(t)) dt.Any other comments?
Two docstring errors in
logistic.pythat I did not touch here, since they are bugs rather than missing formulae, and I would rather not smuggle behaviour-adjacent corrections into a docs PR:F(x) = 1/(1 + exp((x - mu)/s))- the minus sign is missing, so that is the survival function, not the cdf. The code is correct._varat L79 writes\pi^3where the code correctly computespi**2.Happy to fold both into this PR if you would prefer, or I will open a separate
[BUG]issue.On LogNormal, which I also claimed on the issue:
lognormal.pyis the only one of the three files with CRLF line endings, and editing it through the GitHub web editor normalises them to LF, turning a ~60 line addition into a 265-line whitespace diff. Rather than mix a line-ending change into a docs PR, I am leaving it for a follow-up done as a local commit. Worth noting that the CRLF endings in that one file may be unintentional in an otherwise LF repo.