rust-num / num-traits

Numeric traits for generic mathematics in Rust
Apache License 2.0
731 stars 135 forks source link

Add Zero::is_nonzero. #48

Closed clarfonthey closed 6 years ago

clarfonthey commented 6 years ago

Considering how this is a common use case, I figured that having a method for this might be reasonable. It can overridden to be faster in certain cases, but otherwise, it's just a convenience method.

cuviper commented 6 years ago

Can you give an example of a type that would do something other than !self.is_zero()?

clarfonthey commented 6 years ago

@cuviper vector types could simply check self.data.iter().any(Zero::is_nonzero) instead of !self.data.iter().all(Zero::is_zero), and it's not guaranteed that compilers will immediately optimise the latter into the former. On a smaller-scale, Complex is technically a 2D vector and could perform a similar optimisation.

cuviper commented 6 years ago

it's not guaranteed that compilers will immediately optimise the latter into the former.

It's not even obvious which expression would optimize better in the first place, but I'd be really surprised if there was any measurable performance difference.

I also worry that there's a footgun here if someone accidentally implements these not as strict inverses.

clarfonthey commented 6 years ago

The reason why the latter is better is because of the short-circuiting behaviour; it's not clear if the compiler will turn !(x == 0 && y == 0) to x != 0 || y != 0, the latter being faster in every case.

I'd wager that a trivial implementation of quaternions wouldn't be able to make this optimisation, and a program that makes heavy use of them could probably reap the benefits.

clarfonthey commented 6 years ago

As far as the footgun goes, this is already the case with PartialEq, which provides ne for the same reason. Considering how this is basically a zero-specific PartialEq, I'd wager that this is a reasonable optimisation, and that most cases should not overload this method. I could update the docs accordingly if desired.

cuviper commented 6 years ago

The reason why the latter is better is because of the short-circuiting behaviour; it's not clear if the compiler will turn !(x == 0 && y == 0) to x != 0 || y != 0, the latter being faster in every case.

I still don't see it -- && short-circuits false, and || short-circuits true. Any value of x will evaluate these expressions in the exact same way: if x is not zero, it's done, otherwise y is also checked. It's the same with all(is_zero) and any(is_nonzero) -- every input should short-circuit in the same place.

cuviper commented 6 years ago

I still doubt there's any performance difference to be had, so I'm going to reject that basis, barring any concrete examples. I think if there were ever a faster way to write is_nonzero, you could have also written is_zero in that faster form, just negated.

The other question is convenience. In direct use, x.is_nonzero() is longer than !x.is_zero(). It's better as a function argument, like iter.filter(T::is_nonzero) versus iter.filter(|x| !x.is_zero()). But you could say that about almost any method returning bool, yet I think negated forms are uncommon. Collections don't have is_not_empty(), for instance.

(I concede we do have is_even and is_odd in num_integer::Integer, but at least those aren't negated right in the name...)

clarfonthey commented 6 years ago

Sorry for the super delayed response.

Based upon what you say, I'm just going to close this.