dimforge / simba

Set of mathematical traits to facilitate the use of SIMD-based AoSoA (Array of Struct of Array) storage pattern.
Apache License 2.0
290 stars 29 forks source link

Allow direct conversions from f32 to RealField or ComplexField #55

Closed patowen closed 5 months ago

patowen commented 6 months ago

Fixes #54. This should help avoid surprises for users of crates like nalgebra when, for instance, casting f32 matrices to RealField matrices.

One concern that is likely to block a merge of this PR is that anyone who implements RealField and ComplexField would need to implement an additional trait (SupersetOf<f32>). Given that both f32 and f64 have are in frequent use, this might be better long-term, but it would require a major version bump.

I'm not familiar enough with Rust to know whether it's possible to handle this in a backwards-compatible way. As far as I can tell, based on https://github.com/rust-lang/rust/issues/31844, it's not possible to create a default implementation. If I were to write the following

impl<T> SubsetOf<f32> for T where T: ComplexField {
    fn to_superset(&self) -> f32 {
        f64::from_subset(self) as f32
    }

    fn from_superset_unchecked(element: &f32) -> Self {
        f64::from_superset_unchecked(element) as f32
    }

    fn is_in_subset(element: &f32) -> bool {
        f64::is_in_subset(element)
    }
}

it would not compile because it conflicts with the more specific macro in subset.rs that effectively run impl SubsetOf<f32> for f32 and impl SubsetOf<32> for f64.

The right answer might be to keep serde the way it is, although it may be good to document against potential pitfalls that could create. For instance, it hampers nalgebra's cast method for anyone trying to cast an f32 matrix to a RealField matrix. However, if RealField is mostly meant to be used internally by dimforge crates, this might be a non-issue, in which case this PR should almost definitely be closed without merging.

patowen commented 5 months ago

Since this PR has a breaking change and hasn't seen any engagement, I'll go ahead and just close it in favor of keeping any potential discussion in https://github.com/dimforge/simba/issues/54.

sebcrozet commented 3 months ago

Hey! Sorry I haven’t seen that PR earlier. In the future, feel free to keep a PR open, it can take quite some time for me to get around to look at it. I’ve revived these changes in #59

patowen commented 3 months ago

Oh, thanks for looking into this! I partially closed this because I was second guessing myself on this breaking change and was uncertain enough whether this was a good approach that I didn't want to keep an open PR for an extended period of time.

I realize that I don't think I ever used the "request a review" button, so that might explain why you hadn't seen this PR, especially since this library had become relatively mature, no longer needing frequent commits.

If I end up making another PR here for something else, I'll err on the side of keeping it open. Thanks for the heads up!