Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The error message “Domain error in arguments” could be more informative #13172

Merged
merged 4 commits into from
Feb 21, 2022

Conversation

ZhihuiChen0903
Copy link
Contributor

The error message “Domain error in arguments” seems too general to be informative.

C:\ProgramData\Anaconda3\lib\site-packages\scipy\stats\_distn_infrastructure.py in rvs(self, *args, **kwds)
    938         cond = logical_and(self._argcheck(*args), (scale >= 0))
    939         if not np.all(cond):
--> 940             raise ValueError("Domain error in arguments.")
    941 
    942         if np.all(scale == 0):

After reading the source, I believe that the expected behavior is that shape parameters should be positive and scale should >=0. Adding this information to the error message should be more user-friendly.

Expected error message:
Domain error in arguments: shape parameters should be positive and scale should >=0.

@rgommers rgommers added maintenance Items related to regular maintenance tasks scipy.stats labels Dec 1, 2020
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ZhihuiChen0903. A better error message would be nice, but unfortunately this change is not correct - distributions define their own _argcheck that overrides the default one, so it's not just "must be positive". There's no easy way to get this right I'm afraid.

Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>
@mdhaber
Copy link
Contributor

mdhaber commented Feb 21, 2022

No need for this to wait. I don't imagine there will be problems, but I'll re-run CI (close and re-open) and merge if it's green.

@mdhaber mdhaber closed this Feb 21, 2022
@mdhaber mdhaber reopened this Feb 21, 2022
@mdhaber
Copy link
Contributor

mdhaber commented Feb 21, 2022

Might be helpful to use an f-string to mention the name of the distribution (e.g. "see weibull_min documentation"), and perhaps it should be phrased so that it's more clear that scale is not necessarily the problem. But maintainer's requested change has been addressed, lint passes, and this is more informative than it was, so merging.

Thanks, @ZhihuiChen0903! Next time, please create a feature branch instead of using master (now main) for PRs, but thanks for submitting this.

@mdhaber mdhaber merged commit 33f2423 into scipy:main Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants