Skip to content

More consistent error types - #2058

Merged
devmotion merged 5 commits into
masterfrom
dmw/domainerror
Jun 18, 2026
Merged

devmotion merged 5 commits into
masterfrom
dmw/domainerror

Conversation

@devmotion

@devmotion devmotion commented Apr 22, 2026 •

Copy link
Copy Markdown
Member

In a numerical estimation problem, I encountered numerical issues resulting in NaN parameters which then threw a DomainError when constructing a Normal distributions. So fine, so good - I could easily handle it by catching and handling the DomainError gracefully. Only later I noticed that similar numerical problems with Uniform will lead to an ArgumentError.

I think it's more convenient and more predictable for downstream users and packages if incorrect/infeasible parameter values always lead to DomainError.

Changes

  • @check_args now requires every check to name the offending value(s) and always throws a DomainError carrying that value. The previous bare-condition and (cond, message) (ArgumentError) forms are removed; relational checks over several parameters (e.g. a < b) name them with a named tuple such as (; a, b) so downstream catchers can read the individual parameters off err.val.
  • Remaining ad-hoc ArgumentErrors for infeasible parameters were converted to DomainError (or DimensionMismatch where a check is really about sizes/lengths, e.g. MatrixNormal, DiscreteNonParametric).

Merge with master / sparsity tracing

After merging master (which added global sparsity tracing through @check_args via _arg_checkable, #2064), the new named-tuple arguments needed to participate in the tracer-skip decision: a NamedTuple hit the default _arg_checkable(_) = true, so the relational checks still ran on primal-free tracers and errored.

This is fixed by recursing _arg_checkable through Tuple/NamedTuple, so such an argument is checkable only when all of its elements are. Global gradient tracing now works through the real-parameter constructors Uniform, Arcsine, LogUniform, Censored and TriangularDist. (DiscreteUniform and OrderStatistic use the same named-tuple form, but their checked parameters are integer-valued and can never be tracers, so global tracing does not apply to them; the recursion handles their named tuples consistently regardless.)

Because every multi-variable relational check now names all of its variables via a named tuple, the caveat noted in #2067 — that a cross-argument named condition could still slip through — no longer applies anywhere.

Added regression tests covering:

  • global tracing through the affected constructors, including non-dense patterns (minimum/maximum/mode) to confirm dependencies are tracked rather than collapsing to a fully dense Jacobian;
  • the check_args=false bypass under the local detector (primal present, so checks are not auto-skipped) with infeasible values, for both the scalar check (Normal's σ ≥ 0) and the named-tuple check (Uniform's a < b).

Closes #2067.

devmotion and others added 2 commits April 22, 2026 23:42
Distribution constructors now throw a consistent error type based on the
nature of the violation:

* `DomainError(val, msg)` for numerically invalid parameter values. For
  relational checks across multiple parameters (e.g. `a < b` in Uniform,
  `lower ≤ upper` in Censored, `1 ≤ rank ≤ n` in OrderStatistic), the
  named tuple of related parameters is passed as `err.val` so downstream
  catchers can identify each value by name.
* `DimensionMismatch` for structural size checks in constructors — rows/cols
  of `M` vs `U`/`V`/`Σ`/`Ω` in `MatrixNormal`/`MatrixTDist`, and length of
  support vs probability vector in `DiscreteNonParametric`. This matches
  the idiom already used across the library for `suffstats`/`fit_mle`.
* `ArgumentError` is retained only for non-parameter, non-dimension issues
  (empty data in `fit_mle`, invalid enum in `_char_uplo`, etc.).

The `@check_args` macro is collapsed to two `DomainError`-based forms,
`(arg, cond, message)` and `(arg, cond)`, both requiring an offending
value. The previous no-value forms that fell back to `ArgumentError` have
been removed; the macro now errors at expansion time if a caller omits
the value.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Apr 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.63%. Comparing base (4e108fd) to head (a1dd254).

Files with missing lines Patch % Lines
src/utils.jl 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2058      +/-   ##
==========================================
- Coverage   86.65%   86.63%   -0.02%     
==========================================
  Files         149      149              
  Lines        8925     8921       -4     
==========================================
- Hits         7734     7729       -5     
- Misses       1191     1192       +1     

☔ 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 and others added 2 commits June 17, 2026 09:58
)

`@check_args` forwards the explicitly-named argument of each check to
`_arg_checkable` to decide whether the check can run at all; under the global
`TracerSparsityDetector` a primal-free tracer is not checkable, so the check is
skipped and construction can be traced.

Relational checks over several parameters (e.g. `Uniform`'s `a < b`) name them
with a named tuple such as `(; a, b)`. A `NamedTuple`/`Tuple` hit the default
`_arg_checkable(_) = true`, so those checks still ran on tracers and errored.

Recurse `_arg_checkable` through `Tuple`/`NamedTuple` so such an argument is
checkable only when all of its elements are. This fixes global gradient tracing
through the real-parameter constructors `Uniform`, `Arcsine`, `LogUniform`,
`Censored` and `TriangularDist`, with regression tests (including non-dense
patterns to confirm dependencies are tracked, not just that tracing succeeds).

`DiscreteUniform` and `OrderStatistic` use the same named-tuple form but their
checked parameters are integer-valued and can never be tracers, so global
tracing does not apply to them; the recursion handles their named tuples
consistently regardless. Also bump the version.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@devmotion
devmotion requested a review from andreasnoack June 17, 2026 08:48
@devmotion
devmotion merged commit 03ddda7 into master Jun 18, 2026
12 checks passed
@devmotion
devmotion deleted the dmw/domainerror branch June 18, 2026 11:42
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.

Global sparsity tracing still fails for distributions whose @check_args uses bare (non-named-argument) conditions

3 participants