search

statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
544 stars 78 forks source link

Fix simple clippy warnings #216

Closed FreezyLemon closed 2 months ago

FreezyLemon commented 2 months ago

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

YeungOnion commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

FreezyLemon commented 2 months ago

Added some more trivial clippy fixes, some from nightly.

YeungOnion commented 2 months ago

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.