Validate the bin count in FalsificationResult.plot, delegate the rest (#284) - #454
Validate the bin count in FalsificationResult.plot, delegate the rest (#284)#454anevolbap wants to merge 3 commits into
FalsificationResult.plot, delegate the rest (#284)#454Conversation
5164d63 to
2e0daec
Compare
… rest (pymc-labs#284) The `bins < 1` guard let a float like 2.5 through to a matplotlib TypeError, which is what pymc-labs#284 reports. The same guard also raised `TypeError: '<' not supported` on a strategy name or an edge sequence, so neither ever reached matplotlib even though it accepts both. Check only what the bin count owns: reject a non-positive integer, and reject types matplotlib cannot take at all. Strategy names and edge sequences now pass through and matplotlib validates them, which it does more precisely ("'nonsense' is not a valid estimator for `bins`"). Widen the annotation to match what the parameter actually accepts. Add falsification edge-case tests for the bins cases and for a three-variable ~~ block.
2e0daec to
00f5c35
Compare
drbenvincent
left a comment
drbenvincent
left a comment
There was a problem hiding this comment.
Standards
Clean, focused change. The validation logic is well-scoped: pathmc owns the integer bin-count contract, matplotlib owns strategy names and edge sequences. The explicit bool guard is the right call (True subclasses int), and accepting np.integer is a nice touch beyond plain int.
Docstring and type annotation now match actual behaviour. Error messages name the problem and suggest the fix. Tests are parametrized and readable; the shared result fixture avoids repetition without over-abstracting.
One tiny nit (non-blocking): test_invalid_bins_type_rejected matches "bins must be a positive integer", which is a substring of the longer type-error message rather than the exact wording. Fine for now, but a tighter match on the full type-error string would make the test more precise.
Lint passes locally (make lint).
Spec
Closes #284 completely:
plot(bins=2.5)→ clearValueErrorbefore matplotlibbins=0/bins=-1still rejected;None/10still work- Three-variable
~~block test pinned explicitly - Bonus coverage: strategy names, edge sequences,
np.int64, matplotlib's estimator error — all sensible extensions of the same bug surface
The issue's suggested fix (not isinstance(bins, int)) would have rejected valid np.int64 inputs; this implementation is strictly better.
Follow-up
Agree with the PR description: the same bare bins < 1 pattern in refute.py and the unvalidated bins passthrough in interpret.py are worth a separate issue rather than expanding this PR.
Summary: 1 minor nit (Standards), 0 blockers (Spec). LGTM.
Both plot(bins=...) tests matched "bins must be a positive integer", which is a prefix of the type error and the range error alike, so neither test pinned which one actually fired. Anchor each pattern to its own message: the range test on the full string including the parametrized value, the type test on the invariant prefix up to the variable repr.
2 participants
Closes #284.
plot()guardedbinswith a barebins < 1. A float like 2.5 passed that check and failed later inside matplotlib, which is the reported symptom. The same expression also raisesTypeError: '<' not supported between instances of 'str' and 'int'for a binning strategy name, and likewise for a list or tuple of bin edges, so neither ever reached matplotlib even thoughAxes.histaccepts both.The fix checks only what the bin count owns:
ValueError, as beforeAxes.histcannot take at all (float, bool, set) raisesValueErrorDelegating the last case keeps matplotlib's more precise reporting:
bins="nonsense"gives'nonsense' is not a valid estimator for bins, which re-implementing the rules here would lose.The annotation widens to
int | str | Sequence[float] | np.ndarray | Noneto match what the parameter accepts.Tests cover the rejected types, the non-positive counts, the accepted counts including
np.int64, the strategy names and edge sequences that previously raisedTypeError, and matplotlib's estimator error. Also adds the three-variable~~block case from the issue.One follow-up question:
refute.py:395guards its ownbinswith the same barebins < 1, andinterpret.py:214declaresbins: int | Nonebut passes it toAxes.histunvalidated. Worth a separate issue, or out of scope?