Gradient enum and Kernel trait V2 #601

ripytide · 2024-05-10T13:25:49Z

This is a version 2 PR of #596

This time I've added a Kernel trait to allow heap OR stack allocated kernel usage using OwnedKernel and BorrowedKernel respectively. I also rolled back the 1D kernel function back to taking &[K].

Summary:

Made more functions taking 2D kernels the impl Kernel<K> trait for Heap or Stack kernels
Moved HORIZONTAL_SOBEL and similar constants to OwnedKernel::sobel_horizontal_3x3() and made them generic for any `T: From
Removed horizontal_* and vertical_* functions since they can all be achieved via composition such as filter(image, OwnedKernel::sobel_horizontal_3x3()) which is more explicit and rusty.
Bumped MSRV to 1.75.0 for the enumerate() method of Kernel which uses return position impl traits.
Made Kernel::filter a free-standing function which is more consistent with the rest of the library crate::filter::filter()
Added a filter_clamped() function
Removed the filter3x3() function as it is no different from filter_clamped()

cospectrum · 2024-05-10T13:32:54Z

examples/gradients.rs

-    let gradient = map_pixels(&gradient, |_, _, p| Luma([(p[0].abs() / scale) as u8]));
-    gradient.save(&output_dir.join(file_name)).unwrap();
+) {
+    let horizontal_gradients = filter3x3(input, gradient_kernel.kernel1::<i16>());


I don't know why we should change the filter3x3 signature. Just Kernel::filter

I don't know what you mean?

You can just call kernel.filter(input), why do you want to change filter3x3? It's very popular function and will not compile after merge for most users

The reason is because of #520 where users are struggling to discover the filter() method because it is a associated method rather than a free-standing function like every other function in the library. This is very inconsistent and so it would be better to just use filter3x3(image, kernel) or filter(image, kernel) instead of kernel.filter(image).

I think we can do it without breaking changes!
What about pub fn filter<'a>(image: &Image<P>, kernel: &Kernel<'a, K>, f: impl FnMut(&mut Q::Subpixel, K)) -> Image<Q>?

We can also implement Deref, so it's possible to call filter3x3(&img, &kernel)

impl<'a, T> Deref for Kernel<'a, T> { type Target = [T]; fn deref(&self) -> &Self::Target { self.data } }

It's not difficult to find Kernel methods, we just don't have examples. Also there is no clamped_filter method, which is easier to use

I added a filter_clamped() method in this PR for convenience.

pub fn filter<'a>(image: &Image
, kernel: &Kernel<'a, K>, f: impl FnMut(&mut Q::Subpixel, K)) -> Image

This is already possible by the filter() method in this PR

But you have a trait

Yes the trait is used in the filter_clamped() function. Its signature is simply: pub fn filter_clamped<P, K, S>(image: &Image<P>, kernel: impl Kernel<K>) -> Image<ChannelMap<P, S>>

OwnedKernel and BorrowedKernel are the main two types which now implement the Kernel trait.

cospectrum

Breaking changes, and there are only 4 days left before the release.

ripytide · 2024-05-10T13:39:35Z

I'm in no rush to get this merged before the next release, the breaking changes are allowable since we're in pre 1.0 stage, and justified given gradients() and filter() are much more flexible now than before and GradientKernel is more ergonomic in my opinion.

cospectrum · 2024-05-10T14:02:53Z

impl Kernel<'static, i32> {
    pub fn vertical_sobel() -> Self {
        Self::new(&crate::gradients::VERTICAL_SOBEL, 3, 3)
    }
}

cospectrum · 2024-05-10T14:24:30Z

Creating traits and additional Kernel structs is too much for a small issue like #520. I don't think the owner will be convinced, but I have suggested a possible solution.

ripytide · 2024-05-10T15:01:58Z

The point is that consistency is important and having some important methods be free-standing and other associated with types is inconsistent. This PR fixes that. The traits are simply much more flexible as they allow heap OR stack allocated kernels which is necessary for dynamically sized kernels such as for the GradientKernel which is 3x3 or 2x2.

cospectrum · 2024-05-10T15:17:36Z

which is necessary for dynamically sized kernels such as for the GradientKernel which is 3x3 or 2x2

No, it's not

#[derive(Debug, Copy, Clone)]
pub enum Gradient {
    /// The 3x3 Sobel kernel
    /// See <https://en.wikipedia.org/wiki/Sobel_operator>
    Sobel,
    /// The 3x3 Scharr kernel
    Scharr,
    /// The 3x3 Prewitt kernel
    Prewitt,
    /// The 2x2 Roberts kernel
    /// See <https://en.wikipedia.org/wiki/Roberts_cross>
    Roberts,
}

impl Gradient {
    pub fn vertical_kernel(&self) -> Kernel<'static, i32> {
        match self {
            Gradient::Sobel => Kernel::new(&VERTICAL_SOBEL, 3, 3),
            Gradient::Scharr => Kernel::new(&VERTICAL_SCHARR, 3, 3),
            Gradient::Prewitt => Kernel::new(&VERTICAL_PREWITT, 3, 3),
            Gradient::Roberts => Kernel::new(&VERTICAL_ROBERTS, 2, 2),
        }
    }
}

ripytide · 2024-05-10T15:25:00Z

Ok, it's not necessary for the compile-time dynamic GradientKernel enum, but it is necessary for run-time dynamically sized kernels, like for example a cli script that took the kernel size as a command-line argument and then did a convolution based on that kernel size.

How would you implement this?

pub fn guass_kernel(radius: u32) -> BorrowedKernel<'static, i32> {
    //impossible without leaking memory
}

cospectrum · 2024-05-10T15:37:32Z

I would just return Vec<i32> or Image<Luma<T>>.
Maybe you should create an issue and ask @theotherphil about some changes in Kernel api? This way you won't waste your time writing code that won't be accepted.

ripytide · 2024-05-10T15:45:30Z

Vec<i32> is missing dimensions, Image cannot be used with any of the filter functions both are inferior to OwnedKernel + the Kernel trait.

A PR is easier to discuss changes over than an issue as it shows the full extent of a change required.

cospectrum · 2024-05-10T15:53:45Z

Maybe we can add from_image and that's it.

impl<'a, T> Kernel<'a, T>
where T: Primitive
{
    /// Create kernel from image
    pub fn from_image(image: &'a Image<Luma<T>>) -> Self {
        Self::new(image.as_ref(), image.width(), image.height())
    }
}

ripytide · 2024-05-10T16:00:39Z

But that still doesn't allow the implementation of guass_kernel().

pub fn guass_kernel(radius: u32) -> BorrowedKernel<'static, i32> {
    let image = (0..radius).cartesian_product(0..radius);
    BorrowedKernel::from_image(&image) // borrow checker issue
}

cospectrum · 2024-05-10T18:43:40Z

So, the question is whether we want struct OwnedKernel or not.

ripytide · 2024-05-10T19:52:09Z

Why not add it @cospectrum? Owned structs can also be stored in other structs/enums more easily as they don't have leaky lifetimes.

cospectrum · 2024-05-10T21:40:23Z

I don't have any strong arguments for or against. your example make sense, but there is one point: in the library itself there is not yet a single place where we could use this OwnedKernel. That is, it will be an export-only struct.

theotherphil · 2024-05-11T14:05:51Z

Thanks for your work on exploring options to improve filter ergonomics.

Unfortunately these changes add too much user facing complexity for the benefits they bring.

Could the discoverability issue of Kernel::filter be fixed by just making this associated function freestanding?

ripytide · 2024-05-11T14:16:11Z

Could the complexity of using a Kernel trait can be mitigated by adding examples to methods taking kernel arguments? I disagree that the ergonomics and flexibility is not worth the complexity.

Dynamically sized kernels seems to me to be a fairly reasonable usecase, for example, opencv has the ksize parameter (https://docs.opencv.org/4.x/d4/d86/group__imgproc__filter.html#gaabe8c836e97159a9193fb0b11ac52cf1) which would be made much easier to implement using the OwnedKernel.

cospectrum · 2024-05-11T14:36:51Z

I don't really understand what the problem is with the Kernel::filter. 2 ppl in the issie saw filter3x3 function and wanted filter_nxm(image: &GrayImage, n: usize, m: usize, kernel: &[T]), they didn't want OwnedKernel or Kernel trait. And the trait will cause much more trouble to the user than just a structure with a method, btw.
clamped_filter is what they need.

ripytide · 2024-05-12T13:54:32Z

Wait a minute, @cospectrum

No, it's not

#[derive(Debug, Copy, Clone)]
pub enum Gradient {
    /// The 3x3 Sobel kernel
    /// See <https://en.wikipedia.org/wiki/Sobel_operator>
    Sobel,
    /// The 3x3 Scharr kernel
    Scharr,
    /// The 3x3 Prewitt kernel
    Prewitt,
    /// The 2x2 Roberts kernel
    /// See <https://en.wikipedia.org/wiki/Roberts_cross>
    Roberts,
}

impl Gradient {
    pub fn vertical_kernel(&self) -> Kernel<'static, i32> {
        match self {
            Gradient::Sobel => Kernel::new(&VERTICAL_SOBEL, 3, 3),
            Gradient::Scharr => Kernel::new(&VERTICAL_SCHARR, 3, 3),
            Gradient::Prewitt => Kernel::new(&VERTICAL_PREWITT, 3, 3),
            Gradient::Roberts => Kernel::new(&VERTICAL_ROBERTS, 2, 2),
        }
    }
}

This doesn't work for any type T, only i32, whereas my implementation works for any T: From<i8>

ripytide · 2024-05-12T13:55:52Z

You would have to write a constant for every single type which would be super annoying to use.

ripytide · 2024-05-12T14:55:07Z

I've now removed the GradientKernel enum and moved all the Kernel constructors to associated methods of OwnedKernel.

cospectrum · 2024-05-12T17:10:21Z

Wait a minute, @cospectrum

No, it's not

#[derive(Debug, Copy, Clone)]
pub enum Gradient {
    /// The 3x3 Sobel kernel
    /// See <https://en.wikipedia.org/wiki/Sobel_operator>
    Sobel,
    /// The 3x3 Scharr kernel
    Scharr,
    /// The 3x3 Prewitt kernel
    Prewitt,
    /// The 2x2 Roberts kernel
    /// See <https://en.wikipedia.org/wiki/Roberts_cross>
    Roberts,
}

impl Gradient {
    pub fn vertical_kernel(&self) -> Kernel<'static, i32> {
        match self {
            Gradient::Sobel => Kernel::new(&VERTICAL_SOBEL, 3, 3),
            Gradient::Scharr => Kernel::new(&VERTICAL_SCHARR, 3, 3),
            Gradient::Prewitt => Kernel::new(&VERTICAL_PREWITT, 3, 3),
            Gradient::Roberts => Kernel::new(&VERTICAL_ROBERTS, 2, 2),
        }
    }
}

This doesn't work for any type T, only i32, whereas my implementation works for any T: From<i8>

But we don't need any type.

ripytide · 2024-05-12T17:22:34Z

Not yet, but what if users have different image pixel formats: u8, i8, u16, etc.. it would be very wasteful to always cast to i32. Would it not be better if the kernels could be created in any generic format from i8?

cospectrum · 2024-05-12T19:32:14Z

A dynamic cast from one type to another will still be a waste of resources. I would expect more from the library. For example, generate all static arrays using macros.

ripytide · 2024-05-12T19:47:19Z

Despite the fact that kernel creation is incomparably tiny when compared with image convolution? That seems like a heck of performance/complexity tradeoff. A lot of the casts can also be optimized by the compiler since the generic will always be known at compile time.

ripytide · 2024-05-19T11:35:44Z

I'm going to close this and work on a V3 that discards the disagreeable things like the Kernel trait and just focuses on the minimal other things that have been largely agreed on.

cospectrum reviewed May 10, 2024

View reviewed changes

ripytide added 4 commits May 10, 2024 14:33

add the GradientKernel enum

0a315f0

refactor the rest of the codebase to use the new enum

2592950

created kernel traits

2ffecdc

reverted 1D kernel function to just using slices again

5dd2abc

ripytide force-pushed the gradient branch from 84b64f0 to 5dd2abc Compare May 10, 2024 13:34

cospectrum reviewed May 10, 2024

View reviewed changes

ripytide mentioned this pull request May 10, 2024

Gradients, Filter and Kernel Refactor #596

Closed

cargo fmt

a5bde11

ripytide force-pushed the gradient branch from bd9875b to a5bde11 Compare May 10, 2024 13:50

increase msrv to 1.75.0 for return position impl traits

f0ad52a

ripytide force-pushed the gradient branch from 715e506 to f0ad52a Compare May 10, 2024 13:58

replace filter3x3 with more general filter_clamped

d4bdc87

removed the enum and replaced with OwnedKernel constructors

11eb4f0

ripytide force-pushed the gradient branch from f9b3002 to 11eb4f0 Compare May 12, 2024 14:51

reverted the kernel trait

691e551

ripytide closed this May 19, 2024

ripytide mentioned this pull request May 19, 2024

Gradient Refactor V3 #611

Merged

Gradient enum and Kernel trait V2 #601

Gradient enum and Kernel trait V2 #601

Conversation

ripytide commented May 10, 2024 • edited

cospectrum May 10, 2024 • edited

Choose a reason for hiding this comment

ripytide May 10, 2024

Choose a reason for hiding this comment

cospectrum May 10, 2024 • edited

Choose a reason for hiding this comment

ripytide May 10, 2024

Choose a reason for hiding this comment

cospectrum May 10, 2024 • edited

Choose a reason for hiding this comment

cospectrum May 10, 2024 • edited

Choose a reason for hiding this comment

ripytide May 10, 2024

Choose a reason for hiding this comment

cospectrum May 10, 2024 • edited

Choose a reason for hiding this comment

ripytide May 10, 2024

Choose a reason for hiding this comment

ripytide May 10, 2024

Choose a reason for hiding this comment

cospectrum left a comment

Choose a reason for hiding this comment

ripytide commented May 10, 2024

cospectrum commented May 10, 2024 • edited

cospectrum commented May 10, 2024 • edited

ripytide commented May 10, 2024

cospectrum commented May 10, 2024

ripytide commented May 10, 2024 • edited

cospectrum commented May 10, 2024

ripytide commented May 10, 2024

cospectrum commented May 10, 2024

ripytide commented May 10, 2024 • edited

cospectrum commented May 10, 2024 • edited

ripytide commented May 10, 2024 • edited

cospectrum commented May 10, 2024

theotherphil commented May 11, 2024

ripytide commented May 11, 2024

cospectrum commented May 11, 2024 • edited

ripytide commented May 12, 2024 • edited

ripytide commented May 12, 2024

ripytide commented May 12, 2024 • edited

cospectrum commented May 12, 2024

ripytide commented May 12, 2024

cospectrum commented May 12, 2024

ripytide commented May 12, 2024

ripytide commented May 19, 2024

ripytide commented May 10, 2024 •

edited

cospectrum May 10, 2024 •

edited

cospectrum May 10, 2024 •

edited

cospectrum May 10, 2024 •

edited

cospectrum May 10, 2024 •

edited

cospectrum May 10, 2024 •

edited

cospectrum commented May 10, 2024 •

edited

cospectrum commented May 10, 2024 •

edited

ripytide commented May 10, 2024 •

edited

ripytide commented May 10, 2024 •

edited

cospectrum commented May 10, 2024 •

edited

ripytide commented May 10, 2024 •

edited

cospectrum commented May 11, 2024 •

edited

ripytide commented May 12, 2024 •

edited

ripytide commented May 12, 2024 •

edited