-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add clahe operator #2726
Add clahe operator #2726
Conversation
This really just wraps the vips hist_local command, so perhaps the arguments to this operator need to match the valid inputs to vips:
I believe I can generate some sample expected outputs directly with libvips and use those for some unit tests, as demonstrated by something like the unit tests for the I think it also makes sense to update the |
e8194f6
to
43601f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you very much Brad. I've left a few small questions/comments inline that might need addressing but otherwise this is good to merge.
43601f9
to
aca6745
Compare
I believe I addressed your comments with the force push. Thanks for the quick feedback. |
Do types automatically get rebuilt when you do a release or should I include some update to the typings for Typescript in this pull request? |
Hmm, vips actually looks (and behaves, now that I've tried this with non-integers like 0.5 or 2.5) like it only takes integers as the maxSlope, so I'll need to modify the expected type and argument checking of the |
d3b27fb
to
bd80846
Compare
I made these changes, so now the maxSlope should be an int, which matches what the libvips hist_local api accepts. It was just truncating the decimel before, so 0.5 would be the same as 0, and 1.0, 1.1, and 1.9 would all return the same result as if maxSlope = 1. |
Darn, not quite, I missed a few things that make the build fail, and now that I've changed everything, the tests are failing with similarity thresholds that are too high. I'll manually compare the the images to see if they look as they should compared with the libvips outputted images. |
bd80846
to
d2c3f81
Compare
The threshold failing tests were resolved after deleting and rebuilding It should be good now, and I've tested it manually a bit outside the tests to double check. |
Marvellous, thanks again.
These are managed by the https://github.com/DefinitelyTyped/DefinitelyTyped repo. If you're able to add these (after v0.28.3 is released), that would be great. |
I have the patch (since I needed it locally for testing my client code anyway) so I'll push it into a draft pull request over there to time when 0.28.3 is released and keep you in the loop. Thanks for the help getting this in! |
This should fix #1066
but it still needs tests.Is something like this along the right track?
@lovell Do you have any advice on an overall methodology for testing something like this? Should I generate some sample images or something directly with libvips and use those as expected inputs and outputs for a test? What's the standard practice here?