MAINT: stats: more specific error type from rv_continuous.fit
#15778
Conversation
|
Maybe subclass the RuntimeException? |
| @@ -1702,6 +1702,11 @@ def _get_fixed_fit_value(kwds, names): | |||
| ', '.join(repeated)) | |||
| return vals[0][1] if vals else None | |||
|
|
|||
|
|
|||
| class FitError(RuntimeError): | |||
There was a problem hiding this comment.
If we have custom errors, they should be available in the public API so that user can rely on it in their try, except blocks. I don't think we have a central place for these now, but maybe we could have for each module scipy.xxx.exceptions
There was a problem hiding this comment.
This is a good idea, but yes, part of a broader issue. See gh-13462. As noted there, adding FitError to the public API would require approval from the mailing list. Actually, gh-13462 would suggest that instead of FitError itself being public, it should probably be a subclass of a more general public error like SciPyStatsError to minimize the number of separate errors in the API.
Shall we merge this and I'll tackle that issue next?
There was a problem hiding this comment.
@tupui I started looking into this in mdhaber#74. One thought is having a submodule, like you say, but maybeit would be enough to expose just a few top-level warnings/errors as suggested in gh-13462. Anyway, there are some decisions to be made, which is why I'd suggest leaving this with FitError private in this PR, then we can continue working toward the bigger issue.
There was a problem hiding this comment.
Just seeing this issue. Yes it's a good idea to just expose a few errors and do some subclassing. Only drawback of the subclassing is that we need to document what is the base class so users can catch the correct one. Ok to merge now, as you will continue working on this. In the end we just need to ensure we don't advertise for FitError if not part of the API.
| Raises | ||
| ------ | ||
| TypeError, ValueError | ||
| If an input is invalid | ||
| FitError | ||
| If fitting fails or the fit produced would be invalid |
There was a problem hiding this comment.
Should we generalise this? IMO yes, it's good practice.
|
Changes look good. Given that quite a few things can go wrong when estimating the parameters of the distribution, adding |
|
Test failures are unrelated: scipy/stats/tests/test_distributions.py::TestTruncnorm::test_moments |
[skip actions] [skip azp]
|
Yes, most test failures were unrelated but the CircleCI failure doesn't appear to be in |
* master: (632 commits) Update _dual_annealing.py (scipy#15939) TST: stats: make `check_sample_var` two-sided (scipy#15723) DOC: sparse.linalg: add citations for COLAMD DOC: fix missing comma in conf ENH/MAINT: Version switcher from the sphinx theme (scipy#15380) MAINT: stats: remove support for `_rvs` without `size` parameter (scipy#15917) BUG: Handle base case for scipy.integrate.simpson when span along the axis is 0 (scipy#15824) MAINT: stats: adjust tolerance for failing test only MAINT: stats: adjust tolerance of failing TestTruncnorm MAINT: special: Clean up C style in ndtr.c CI: remove pin on Jinja2 (scipy#15895) STY: Remove white spaces MAINT: stats: exempt gilbrat from refguide_check MAINT: stats: rename continuous_gilbrat->continuous_gibrat MAINT: stats: correct name Gilbrat -> Gibrat [ENH] circvar calculated simply as 1-R (scipy#5747) DEP: deal with deprecation of ndim >1 in bspline MAINT: stats: more specific error type from `rv_continuous.fit` (scipy#15778) DOC: fix import in example in _morestats (scipy#15900) [BUG] make p-values consistent with the literature (scipy#15894) ...
Reference issue
Closes gh-14505
What does this implement/fix?
When
rv_continuous.fitfails inmain, it raises a plainException. This is not as specific as it should be. This PR changes it to aRuntimeErroras suggested in gh-14505, but ideas for an even more specific exception type are welcome.