search

image-rs / imageproc

Image processing operations
MIT License
736 stars 144 forks source link

Rebased and Refactored #471 + `range()` function #587

Closed ripytide closed 3 months ago

ripytide commented 4 months ago

I'm working through testing/reviewing all the PRs currently.

This PR is a rebase+refactor of #471. #471 seems like a fairly standard and objective image processing function. I've refactored it to use map() as well as adding a range() function to the stats.rs module which is used in normalize_linear().

I also refactored the tests and added benches for the new normalize functions.

Merging this would close #471 and #470.

theotherphil commented 4 months ago

It looks like you’re missing the change to actually use range in normalise_linear.

Range is a bit of a confusing name here as users will think of the standard library’s Range types. Maybe MinMax?

The existing stretch_contrast and the new normalize_linear are both special cases of a function that specifies both the desired input and output ranges so I’d like them to be unified rather than adding a similar-but-different parallel implementation.

theotherphil commented 4 months ago

Thanks for the work reviewing and updating old PRs :-)

ripytide commented 4 months ago

I didn't see stretch_contrast() but you're right it's very similar. I've combined both normalize_linear() and stretch_contrast() into scale_linear() which takes both the input_min/max and output_min/max which should allow the user to easily accomplish either goal.

range() and Range have been renamed to minmax() and MinMax to prevent confusion with std range types as suggested.

I've also removed normalize_nonlinear() as it used f64 and was quite complicated and I don't think many users are likely to use it so it is best left up to the users that do want it to implement themselves.

ripytide commented 3 months ago

Can I ask why the unused_imports lint is allowed? I likely would have caught them before committing had the compiler told me they existed.

theotherphil commented 3 months ago

Good question. It was added in https://github.com/image-rs/imageproc/pull/549 and I didn’t spot it at the time. cc @cospectrum

theotherphil commented 3 months ago

Thanks!

cospectrum commented 3 months ago

Good question. It was added in #549 and I didn’t spot it at the time. cc @cospectrum

Because there are many places with unused imports, and it will take a lot of time to remove them away

theotherphil commented 3 months ago

Seems easy enough to fix - I'll create a PR to remove them.