Skip to content

Fix fit_mle dispatch for instantiated/concrete MvNormal aliases#2078

Open
rsenne wants to merge 1 commit into
JuliaStats:masterfrom
rsenne:master
Open

Fix fit_mle dispatch for instantiated/concrete MvNormal aliases#2078
rsenne wants to merge 1 commit into
JuliaStats:masterfrom
rsenne:master

Conversation

@rsenne

@rsenne rsenne commented Jul 3, 2026

Copy link
Copy Markdown

Fixes #2074.

This widens the 7 fit_mle signatures to D::Type{<:...} so the aliases are
accepted in any form (bare, instantiated, or concrete member), restoring the
pre-#2073 behavior. Added tests cover the instantiated and concrete-type paths,
which the existing bare-alias tests didn't exercise.

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.74%. Comparing base (2697768) to head (c563676).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2078   +/-   ##
=======================================
  Coverage   86.74%   86.74%           
=======================================
  Files         149      149           
  Lines        8883     8883           
=======================================
  Hits         7706     7706           
  Misses       1177     1177           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@devmotion

devmotion commented Jul 3, 2026

Copy link
Copy Markdown
Member

I'm not even sure this should be considered a bug and be "fixed". The loose fit_mle methods that accept arbitrarily parameterized types exist for historical reasons presumably but as noted in the documentation they are completely unfaithful as generally you won't obtain a distribution of the provided type. Compare this eg with convert(T, x) which is expected to return an instance of type T. So IMO one should only ever use - and arguably only support - fit_mle(Normal, xs), fit_mle(Beta, xs), fit_mle(IsoNormal, xs) etc because these actually return distributions of type Normal, Beta, IsoNormal etc. but not fit_mle(Normal{Float32}, xs), fit_mle(Beta{Float64}, xs) or more generally fit_mle(typeof(d), xs) as these generally do not return distributions of type Normal{Float32}, Beta{Float64} or typeof(d).

To summarize, I think the better as more faithful approach is to not support fit_mle(::Type{<:IsoNormal}, ...) etc. but only fit_mle(::Type{IsoNormal}, ...) etc.

@rsenne

rsenne commented Jul 3, 2026

Copy link
Copy Markdown
Author

I agree on the long-term design: fit_mle(Normal{Float32}, xs) returning a Float64 distribution is unfaithful--something this PR admittedly overlooks--and given the current design the API probably shouldn't encourage passing fully-parameterized types.

But I'd like to separate that design question from what I think is the real issue: compatibility. Generic code that refits distributions with fit(typeof(d), data) worked before 0.25.129, at least in the Float64 case. It now errors, which is a behavioral regression and the error message (suffstats is not implemented) doesn't make the cause obvious.

So I think the best middle ground is to reinstate the previous behavior but emit a deprecation warning, which addresses your concern while not breaking existing code in a patch release. That restores what worked, signals the intended direction, and gives downstream users a clear message instead of a cryptic error.

@rsenne

rsenne commented Jul 4, 2026

Copy link
Copy Markdown
Author

Alternatively --I took a crack at making the fitting path type-generic. Fitting now works for any real eltype instead of only Float64, and returns a distribution matching the input's element type -- which I think fits what you wanted on the faithfulness side: fit(typeof(d), data) gives you back d's type rather than coercing to Float64.

Kept the Float64 BLAS.syrk path as-is; the generic methods are just fallbacks for the eltypes that used to error. Full suite passes locally. So i think this would preserve the old behavior.

As a note I also did something similar for the univariate case.

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.

fit/fit_mle for DiagNormal/IsoNormal fails: "suffstats is not implemented"

2 participants