Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added manual threshold algorithm #56

Merged
merged 8 commits into from
Jan 12, 2023

Conversation

volks73
Copy link
Contributor

@volks73 volks73 commented Jan 7, 2023

Simple implementation to apply thresholding with manual lower and upper limits. The auto-thresholding algorithms can be used to automatically select a threshold and apply it, but a function to simply apply user determined limits could be useful.

@volks73
Copy link
Contributor Author

volks73 commented Jan 8, 2023

I forgot to add a unit test for the "manual" threshold. However, in going back and implementing the unit test, I realized the "manual" algorithm is just an application of a calculated or determined threshold value. So, I changed the ThresholdManualExt trait to ThresholdApplyExt and simplified a little bit by combining the range feature with the existing apply_threshold private function.

I also refactored the imports for the development to be within the tests sub-module.

Something to consider is renaming the threshold_otsu and threshold_mean functions for the ThresholdOtsuExt and ThresholdMeanExt traits, respectively. I would suggest breaking them into two separate functions, one that calculates the threshold level and the other that calculates and applies, for example:

...
fn threshold_calculate_otsu(&self) -> Result<f64, Error>;

fn threshold_apply_otsu(&self) -> Result<Self::Output, Error>;
...

The threshold_apply_otsu would be the current implementation, just a renaming to be clear it is applying the threshold as calculated by the Otsu method, while the threshold_calculate_otsu returns the calculated threshold value that could be used with the threshold_apply function or for further processing/algorithms.

@xd009642
Copy link
Member

Sure I'm fine with splitting threshold calculation and application 👍. PR overall looks good so far, if you also update the changelog adding a new [Unreleased] field at top and noting down any changes (I won't review that too stringently it's more so next release I don't forget anything done when finalising changelog)

@volks73
Copy link
Contributor Author

volks73 commented Jan 12, 2023

I have added the [unreleased] section to the CHANGELOG.md file.

Separating out the apply from the calculate as different methods should be addressed in a separate PR since it could potentially break compatibility with previous versions.

@xd009642
Copy link
Member

Sure, if you want to make an issue to track it or just go straight into the PR if you want to I'll just give this a last looksie and then merge if it still looks all good

@xd009642 xd009642 merged commit 5223a69 into rust-cv:master Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants