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

Fix simple clippy warnings #216

Merged
merged 7 commits into from
Apr 21, 2024

Conversation

FreezyLemon
Copy link
Contributor

There are a few more clippy warnings, but those aren't as clear-cut as the ones I fixed in this PR.

@YeungOnion
Copy link
Contributor

Thanks!

Any thoughts on how to approach the one in uniform? I don't think uniform distribution is very well defined over an infinite (or semi-infinite) interval, but I'm not a formal mathematician.

If you agree, I'll ensure boundedness in new and drop the additional checks.

After all, calls to our sample would fail in UniformFloat::new in the rand crate and there's not a way to define inverse_cdf.

@FreezyLemon
Copy link
Contributor Author

FreezyLemon commented Apr 19, 2024

Any thoughts on how to approach the one in uniform?

I think the logic itself is fine, clippy just doesn't like that there are two else if branches because that could be done in one:

} 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 #[allow] attribute.

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.

@YeungOnion
Copy link
Contributor

Regarding the logic, is it not redundant?

If the first case is false, then self.min > NEG_INF and x > NEG_INF
If the third is true, then x = INF and self.max is either NEG_INF (implies x>) or INF (implies x=)

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.

@FreezyLemon
Copy link
Contributor Author

Regarding the logic, is it not redundant?

If the first case is false, then self.min > NEG_INF and x > NEG_INF If the third is true, then x = INF and self.max is either NEG_INF (implies x>) or INF (implies x=)

Both options would have satisfied the second, no?

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 else if self.max.is_infinite() below that is also redundant, but I can't be bothered to do the logic in my head right now :P

Can you share what you think that use case might be? I'll take a chance to expand my brain.

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.

@YeungOnion
Copy link
Contributor

Okay, I'll fix that separately since it's a behavior change instead of linting.

@FreezyLemon
Copy link
Contributor Author

Added some more trivial clippy fixes, some from nightly.

@YeungOnion
Copy link
Contributor

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 ordered_float's NotNan<T> has settled since Empirical was introduced; perhaps we can let clippy allow it for merging CI.

@YeungOnion YeungOnion merged commit 594bced into statrs-dev:master Apr 21, 2024
2 checks passed
@FreezyLemon FreezyLemon deleted the fix-simple-clippy-lints branch April 22, 2024 14:04
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.

2 participants