Define partype on types, mirroring eltype#2072
Open
devmotion wants to merge 5 commits into
Open
Conversation
`partype` could previously only be called on instances: every distribution
defined `partype(d::Dist{T}) = T` with a `Float64` fallback, so `partype(SomeDist{T})`
on the type fell through to the default and returned the wrong answer.
Mirror the `eltype` design: define `partype` on the type and provide an instance
method `partype(d) = partype(typeof(d))` for convenience, documented with a
`!!! note` callout. The generic default becomes `Real` (parameters are `<:Real`
and `eltype(Real) === Real`, so `zero`/`one` work on it).
- Migrate all distribution methods from `partype(d::Dist{T})` to
`partype(::Type{<:Dist{T}})`; wrappers recover the wrapped distribution from
their type parameter, as the matching `eltype` methods already do.
- Give distributions that relied on the fallback an explicit method, fixing a
latent bug where parametric distributions (`Dirac`, `DiscreteNonParametric`,
`Product`, `UnivariateGMM`) reported `Float64`. Parameterless distributions
(`Chernoff`, `Kolmogorov`) return `Union{}` (the empty parameter promotion);
`KSDist`/`KSOneSided` return `Int`, `Soliton` returns `Float64`.
- Fix `-(d::UnivariateDistribution)` to `-one(promote_type(Int, partype(d))) * d`
so it stays well-typed when `partype(d) === Union{}`.
- Add tests, including a general invariant in `testutils`:
`partype(d) === partype(typeof(d)) === mapreduce(recursive_partype, promote_type, params(d); init=Union{})`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2072 +/- ##
==========================================
+ Coverage 86.74% 87.40% +0.65%
==========================================
Files 149 149
Lines 8882 8891 +9
==========================================
+ Hits 7705 7771 +66
+ Misses 1177 1120 -57 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`partype` defined on types mirrors `eltype`: a specific method like
`partype(::Type{<:Normal{T}}) where {T<:Real}` only matches a concrete
subtype, so a bare UnionAll (`Normal`, whose parameter is only an upper
bound) does not match and dispatch falls through to the generic
`partype(::Type{<:Distribution}) = Real` default, just like `eltype(Vector)`
returns `Any`.
This was correct but untested. Add assertions covering bare parametric
UnionAlls, abstract nodes in the type hierarchy, and the motivating
`partype(eltype(container))` use for both concrete and abstract element
types, so a future subtyping change can't silently regress the fallback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the `partype` tests out of `test/utils.jl` and into each distribution's own test file, covering the methods that the standard `test_params` battery does not exercise. This fixes the patch-coverage gap for the newly type-based `partype` methods on `BernoulliLogit`, `Semicircle`, `PoissonBinomial`, `Soliton`, the deprecated `Product`, and the unary negation `-(d::UnivariateDistribution)`. - Enable `bernoullilogit`, `hypergeometric`, `ksdist`, and `ksonesided` in `runtests.jl`; the first was never run, the latter three had no test file (they are not part of the ref-data battery), so add minimal files for them. - Keep the generic-mechanism tests (the `Real` fallback for abstract types and `partype(eltype(container))`) in `normal.jl`. - Wrap every `partype` test in `@inferred` so the type-based design is asserted to stay constant-foldable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Motivation
In a downstream package I currently maintain an internal
_partypethat operates on distribution types, so that I can compute the parameter element type of a container of distributions from itseltype— without having to rely on the container being non-empty (i.e. without needing an instance). That internal version is necessarily incomplete: it reimplements per-distribution knowledge that already lives here and can't realistically cover every distribution.The reason it's needed at all is that
partypeis currently only defined on instances: every distribution definespartype(d::Dist{T}) = T, with aFloat64fallback, so there's no way to askpartype(Normal{Float32})for a type — it falls through to the default and silently returns the wrong answer.This PR makes
partypefollow the same design aseltype: define it on the type, with the instance method provided for convenience. That makes the type-level query work directly and lets the downstream_partypego away.Changes
partypeis now defined on the type, withpartype(d) = partype(typeof(d))forwarding for convenience, documented with a!!! notecallout mirroringBase.eltype's docstring.partype(d::Dist{T})topartype(::Type{<:Dist{T}}). Wrapper distributions (Censored,Truncated,AffineDistribution,ReshapedDistribution,OrderStatistic,JointOrderStatistics,MixtureModel,MvLogitNormal) recover the wrapped distribution from their type parameter, exactly as the matchingeltype(::Type{...})methods already do.Float64toReal: distribution parameters are<:Realandeltype(Real) === Real, soRealis the natural fallback (andzero/onework on it, unlikeAny).Dirac,DiscreteNonParametric,Product,UnivariateGMM) reportedFloat64instead of their actual parameter type. Parameterless distributions (Chernoff,Kolmogorov) returnUnion{}— the empty parameter promotion, which is the identity forpromote_typeand so composes correctly through wrappers;KSDist/KSOneSidedreturnInt(theirn),SolitonreturnsFloat64.-(d::UnivariateDistribution)is changed from-one(partype(d)) * dto-one(promote_type(Int, partype(d))) * dso it stays well-typed whenpartype(d) === Union{}(the multiplier-1is anInt, andpromote_type(Int, Union{}) === Int).Behavior changes
partype(Dirac{Float32})etc. now return the real parameter type (Float32) instead ofFloat64.partype(Kolmogorov())/partype(Chernoff())returnUnion{}(wereFloat64). One consequence:-Kolmogorov()now produces aLocationScale{Int,...}rather than silently aFloat64-typed one.Real(wasFloat64).Tests
A general invariant wired into
test/testutils.jl'stest_params, so every distribution that runs the standard battery asserts:partypetests live in each distribution's own test file (instance and type-based calls, instance forwarding, wrappers, the bug-fixes, and theReal/Union{}cases), covering the methods the standard battery does not exercise. To run all of them this:univariate/discrete/bernoullilogitinruntests.jl(the file existed but was never included);Hypergeometric,KSDist, andKSOneSided, which have no entry in the ref-data battery.The generic-mechanism regression tests live in
test/univariate/continuous/normal.jl: theRealfallback for bare parametric UnionAlls (partype(Normal)) and abstract hierarchy nodes (partype(UnivariateDistribution)), plus the motivatingpartype(eltype(container))for both concrete and abstract element types. A bare UnionAll's type parameters are only upper bounds, so the specificType{<:Dist{T}}methods don't match and dispatch falls through to theRealdefault — exactly aseltype(Vector) === Anyin Base. These lock that behavior in so a future subtyping change can't silently regress it.Every
partypetest is wrapped in@inferred, asserting the type-based design stays constant-foldable.The full test suite passes (4,941,004 passing, 0 failures).