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

median filter #1161

Merged
merged 4 commits into from
Mar 17, 2018
Merged

median filter #1161

merged 4 commits into from
Mar 17, 2018

Conversation

BiancoA
Copy link
Contributor

@BiancoA BiancoA commented Mar 16, 2018

following #1159 I added the median filter using vips_rank()

@coveralls
Copy link

coveralls commented Mar 16, 2018

Coverage Status

Coverage increased (+0.009%) to 98.994% when pulling 7ac0447 on BiancoA:master into 48c5f86 on lovell:master.

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Andrea, this will make a great addition and it's always a pleasure to see unit tests in a PR. I've left a couple of comments inline that will need addressing.

src/pipeline.cc Outdated

// Median - must happen before blurring, due to the utility of blurring after thresholding
if (shouldApplyMedian) {
image = sharp::Median(image, baton->medianSize);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably use libvips' median operation, something like:

if (baton->medianSize > 0) {
  image = image.median(baton->medianSize);
}

Copy link
Contributor Author

@BiancoA BiancoA Mar 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, although I am missing something... do we still need

 VImage Median(VImage image, int const m) {
      return image.rank(m, m, (m*m)/2);
}

in src/operations.cc ?

src/pipeline.cc Outdated
@@ -1194,6 +1199,7 @@ NAN_METHOD(pipeline) {
baton->flatten = AttrTo<bool>(options, "flatten");
baton->negate = AttrTo<bool>(options, "negate");
baton->blurSigma = AttrTo<double>(options, "blurSigma");
baton->medianSize = AttrTo<double>(options, "medianSize");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The size is integral so uint32_t would make a better type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed! sorry - again - for the copy-paste.

/*
* Median filter with mask size m
*/
VImage Median(VImage image, int const m) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, do we still need this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot, yes, this code is no longer used. Please go ahead and remove it and its definition in the header file, thank you.

@lovell lovell merged commit 875937e into lovell:master Mar 17, 2018
@lovell
Copy link
Owner

lovell commented Mar 17, 2018

Thank you, this will be in v0.20.1.

@lovell lovell added this to the v0.20.1 milestone Mar 17, 2018
lovell added a commit that referenced this pull request Mar 17, 2018
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.

3 participants