-
Notifications
You must be signed in to change notification settings - Fork 85
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
Fix simple clippy warnings #216
Fix simple clippy warnings #216
Conversation
Thanks! Any thoughts on how to approach the one in If you agree, I'll ensure boundedness in After all, calls to our |
I think the logic itself is fine, clippy just doesn't like that there are two } else if x >= self.max || x.is_infinite() && self.max.is_infinite() { That's less readable though, so I would just keep it as-is and add an P.S.: I should add that I'm not a mathematician either. If you think handling infinite bounds doesn't make sense, it's feasible to change the API to reject that earlier. But I can maybe see some edge-case usage there. |
Regarding the logic, is it not redundant? If the first case is false, then Both options would have satisfied the second, no? Can you share what you think that use case might be? I'll take a chance to expand my brain. |
You're right. The other cases already neatly handle bounds being +/- infinity. I only skimmed the code because clippy is complaining about something else, but the entire block can be removed AFAICT. Makes me wonder if the
Well, my original argument was going to be about API flexibility and that even if it might not be mathematically defined, it could still be useful to "just handle it" transparently instead of erroring. That said, it does look like the uniform distribution requires finite bounds from what I can find online. |
Okay, I'll fix that separately since it's a behavior change instead of linting. |
Added some more trivial clippy fixes, some from nightly. |
This indirectly removes references to deprecated constants
clippy complained about this and thought it was unintended
6216d7b
to
feeb0fd
Compare
Think this is all of them that we can handle as lints then, right? Merging, then I'll put in the change for Uniform. Not sure how I want to handle that one in Empirical. It does seem that |
There are a few more clippy warnings, but those aren't as clear-cut as the ones I fixed in this PR.