Converting from num::Real and Scalar types to f32/f64

[okaneco]

While porting a crate to palette 0.7, I had some difficulty figuring out how to convert to a concrete float type. This seems like it'd be hard for beginner-to-intermediate users to figure out.

This was the solution I ended up with for the trait bounds, constraining to IntoScalarArray<1> + FromScalar<Scalar = f32>.

pub fn difference<Wp, T>(c1: palette::Lab<Wp, T>, c2: palette::Lab<Wp, T>) -> f32
where
    T: palette::num::Real
        + palette::num::Arithmetics
        + palette::color_difference::EuclideanDistance
        + palette::num::IntoScalarArray<1>
        + palette::num::FromScalar<Scalar = f32>
        + Copy,
{
    T::into_array(
        palette::color_difference::EuclideanDistance::distance_squared(c1, c2)
    )[0]
}

Is there a better way to do this? I was wondering if it made sense to have something similar to num-traits's ToPrimitive::to_f64 to correspond with the Real::from_f64 (the return type on ToPrimitive is an Option<f64>, I don't think it'd be a problem to return f64). Or it might just need to be documented somewhere so users can have an easier time finding out how to do this.

[Ogeon]

Hmm, I think the issue here is just that you are trying to convert T to f32. That would depend on what T is and Palette doesn't assume much at all (only the operations for EuclideanDistance in this case). If you don't need SIMD, you can require it to implement Into<f32>, ToPrimitive or any other option that fits your use. The traits in palette::num shouldn't conflict with num_traits, for example, so it's fine to use it in addition. I suppose you could also require EuclideanDistance<Scalar = f32>, but that's practically locking T to f32.

[Ogeon]

I'm thinking something like this should work if you also want to convert T to f32 for the output:

pub fn difference<Wp, T>(c1: palette::Lab<Wp, T>, c2: palette::Lab<Wp, T>) -> f32
where
    T: num_traits::ToPrimitive,
    palette::Lab<Wp, T>: EuclideanDistance<Scalar = T>, // Specifying Scalar may be unnecessary.
{
    c1.distance_squared(c2).to_f32().unwrap() // Or other error handling.
}

[Ogeon]

You are generally right, though, that writing generic code with Palette is under-documented. It may need to be clarified that it's fine to apply more trait bounds and that the traits in palette::num are essentially minimum requirements for maximum generalization. 🤔

[okaneco]

I appreciate the answers.

The interface designs are from when I was somewhat new to Rust so there are things I'd like to change; I wanted to make it easier for users and I wasn't the most confident with generics at the time. I was implicitly relying on Lab<Wp> since palette 0.5 which masked some of the issues since it defaulted to f32. Upgrading to 0.6 was uneventful, but 0.7 exposed the mistaken assumptions. I ended up with the code above because but I wanted to upgrade to 0.7 without majorly changing the public interface. The only user facing changes they should have to make are converting from Pixel::{into_raw, from_raw_slice, into_raw_slice} to the cast equivalents which I can easily cover in the change log.

An issue was filed okaneco/kmeans-colors#51 and it wasn't obvious how to upgrade to the new version because of the flawed assumptions I'd already made (such as implicitly relying on Copy and PartialOrd in some places). I was able to work on it again after porting a different crate with a fresh set of eyes. This gave me a better idea of what I was doing, and brought me to an understanding that echoes your words.

It may need to be clarified that it's fine to apply more trait bounds and that the traits in palette::num are essentially minimum requirements for maximum generalization.

I tried to make everything work with only the palette::num traits at first, but soon realized that wouldn't be possible with what was available. That was a faulty assumption on my part since I was under the impression it was still a wrapper around Float, and I was hesitant to add more traits.

Do you have a recommendation of when users should prefer num-traits versus palette::num? Reading the documentation more, it seems like unless you're using wide, you should probably be using the num-traits instead. Making that more clear or with cookbook examples would be helpful here.

[Ogeon]

I see, sorry for the headaches. First, let me just answer your last question: prefer num-traits over palette::num as much as you possibly can for now. It's much more complete and long-term stable, and also more integrated in the general ecosystem. palette::num should ideally be a separate crate but I didn't want to take that step without trying it out in practice first (and I'm not sure I should be the one maintaining it, honestly).

For the case with Lab (I found your PR and had a look), since you effectively required Lab<Wp, f32> before, you should be able to continue doing that and not make it generic over the component type for now. You shouldn't have to introduce new requirements in that case, but it may be wise to make the f32 explicit. I'm sometimes wondering if it was a mistake to make the component type a default, to be honest.

But if you do want to make it fully generic, I would suggest sticking to num-traits, as mentioned, and use as many high level palette trait bounds as possible, rather than restricting the type T to make them work. So Lab<Wp, T>: FromColor<LinSrgb<f32>>, Lab<Wp, T>: EuclideanDistance, and similar. It's easier to maintain and read in general. Think of palette::num as an implementation detail. You would also allow a larger set of types T and any bounds on it from palette should be valid for f32 too. What would happen, though, is that you start exposing details, such as which distance formula you are using, but that's the unfortunate nature of generic math code in Rust. 😬 It's hard to know where to put the requirements.

I hope this helped putting you back on track, but let me know if not. I'm happy to help where I can and I could also comment with suggestions in the PR if you want.

[okaneco]

Thanks, that does help put me back on course and give me a direction to aim towards. I'll try to re-approach it with Lab<Wp, f32> and/or num-traits if necessary. If I have more questions I'll be sure to post here or ping for comments at that PR.

What would happen, though, is that you start exposing details, such as which distance formula you are using, but that's the unfortunate nature of generic math code in Rust. 😬 It's hard to know where to put the requirements.

Yes, a constant struggle 😅

[okaneco]

I went the num-traits route and was able to finish with relative ease. The main files of interest are src/colors/kmeans.rs and src/colors/sort.rs.
okaneco/kmeans-colors@e61e081

I decided to go generic over the component type and these were the bounds I arrived at. I also changed from Srgb to Rgb to get away from the implied/default values of the color type and the sometimes weird inference errors because of Srgb being an alias.

impl<Wp, T> Calculate for Lab<Wp, T>
where
    T: Float + FromPrimitive + Zero,
    Lab<Wp, T>: core::ops::AddAssign<Lab<Wp, T>> + Default,

impl<S, T> Calculate for Rgb<S, T>
where
    T: Float + FromPrimitive + Zero,
    Rgb<S, T>: core::ops::AddAssign<Rgb<S, T>> + Default,

The reminder about using higher level bounds was very helpful because it allowed me to remove unnecessary implementation traits. I originally had S: RgbStandard and T: IntoStimulus<f32> but they were replaced by an IntoColor<Luma<S, T>> bound for a function that sorted values by brightness after converting to Luma.

impl<S, T> Sort for Rgb<S, T>
where
    T: Float + FromPrimitive + Zero,
    Rgb<S, T>: core::ops::AddAssign<Rgb<S, T>> + IntoColor<Luma<S, T>> + Default,

Thanks for helping me resolve the confusion I originally had 🙂

[Ogeon]

Nice! Looks like you found ways to simplify some of the code, too. 🎉 It makes sense to generalize Rgb as well, since Lab takes basically anything, and it should allow a wider range of image formats too.

If I may come with a small suggestion, you could check if into_array can be replaced with .into(). It should work if the type inference understands it. Unless you prefer into_array, of course! The other cast functions will also have trait/method counterparts in an upcoming version (implemented as ComponentsInto etc.), but I have some more I want to add first so don't wait.

The reminder about using higher level bounds was very helpful because it allowed me to remove unnecessary implementation traits.

That's great! I have become dangerously fond of it, myself, since it's much more "semantic" and usually makes the intentions quite clear. It's just too bad that the error messages still tend to dip down into the detailed bounds of the implementations. Maybe it's hard to know which level to put it on. I know it's possible to customize the error message on nightly, but that's not stable...

[okaneco]

Thanks, I was able to remove into_array and simplify with into(); in another crate, I was able to remove into_array_ref and use into(). I often forget about using into() because of inference not always working.

ComponentsInto looks like a really nice convenience feature, I look forward to being able to use it.