matthewwithanm / pilkit

Utilities and processors built for, and on top of PIL
BSD 3-Clause "New" or "Revised" License
196 stars 54 forks source link

Create GaussianBlur processor #33

Closed kravemir closed 5 years ago

kravemir commented 5 years ago

This is a follow up to #32. The Gaussian Blur requires pre-multiplied alpha mode in order to perform alpha blending/compositing correctly.

For this processor, I have no idea how to test it,.. Advice is welcome.

fish2000 commented 5 years ago

While you wait, you might enjoy some of my own blur-related processors, as well as other Gaussian utility kernels, available in Instakit (which is pip-installable!)

kravemir commented 5 years ago

@fish2000, thank's for suggestion, but I prefer pilkit due to:

    install_requires=[],

But, instakit has more dependencies:

    'Cython>=0.29.0',
    'Pillow>=3.0.0',
    'numpy>=1.7.0',
    'scipy>=1.1.0',
    'scikit-image>=0.10.0']

BUT, unmaintained project screams: NO-GO,... Therefore I might consider instakit, even if I focus on lightweight solutions. However, would it be possible to split instakit into instakit-core with Pillow as only dependency, and rest into instakit-extras / instakit-transforms / idk-relevant-name,.. ?

matthewwithanm commented 5 years ago

@vstoykov is maintaining it. There just isn't much to do since it's pretty stable. (:

fish2000 commented 5 years ago

@kravemir I can vouch for @matthewwithanm’s assertion – geologically speaking, pilkit is the lapis lazuli of Python software projects: stable, rare, and valuable within its ecosystem. In the era of “move fast and break things”, stumbling across such a project must be far from normative – but its dependability is orthogonal to its push frequency.

Incidentally I have been curious as to how the myriad heavy-duty dependencies Instant demands of its putative users have impacted its perception – but frankly, it is still very much an experiment, despite the long-standing stability of many of its components and my commitment, as its author, to not fucking things up once they have been published in an observably unfucked state. I will take your suggestion of spitting it up under advisement; many of the utils components and several processing modules would work fine on their own at this point.

That would, at least, have a deflationary effect on the dependency escalation, indeed. In any case, thanks for weighing in. Yes!

vstoykov commented 5 years ago

@kravemir thank you for your contribution. It's true that currently I do not have too much spare time but pilkit is doing what it is designed for. There are not so many issues (actually currently there is only one, others are proposals for new features).

Now to the point of your PR. I'm also confused about the error from the CI. It can be related to something new in Pillow which is not very good and need to be investigated. About the GaussianBlur I'm not so familiar how it works and how it should be used but when I look at instakit GaussianBlur fliter accepts more arguments and if something should be added as general purpose library should be flexible enough and with stable API. Currently the proposed implementation is simple enough for someone to just include it in his project without using some 3th party library. I'm willing to accept it if it is looking more general purpose, has tests (which I'm not sure how should look for that filter).

About your proposal to @fish2000 of spiting instakit I think that I can improve that proposal if @fish2000 is happy with that to contribute some of the processors which does not depend on 3th party libraries to pilkit for people which do not want too many dependencies. If he is not willing to do it someone else can do it because the license of instakit is allowing it (right?).

And a little bit off topic @matthewwithanm what do you thing about new GituHub organization for django-imagekit and pilkit in order to be easier of giving some administrative permissions and/or giving permissions to new maintainers?

matthewwithanm commented 5 years ago

@vstoykov sounds good to me!

kravemir commented 5 years ago

@vstoykov is maintaining it. There just isn't much to do since it's pretty stable. (:

@kravemir thank you for your contribution. It's true that currently I do not have too much spare time but pilkit is doing what it is designed for. There are not so many issues (actually currently there is only one, others are proposals for new features).

Sounds good. Then, it's just not developed anymore, but maintained (as stable) :- )

About the GaussianBlur I'm not so familiar how it works and how it should be used but when I look at instakit GaussianBlur fliter accepts more arguments and if something should be added as general purpose library should be flexible enough and with stable API. Currently the proposed implementation is simple enough for someone to just include it in his project without using some 3th party library. I'm willing to accept it if it is looking more general purpose, has tests (which I'm not sure how should look for that filter).

@vstoykov I don't know how instakit's GaussianBlur is implemented. But signature of PIL's filter is PIL.ImageFilter.GaussianBlur(radius=2).

The goal is to make PIL's GaussianBlur filter available as pilkit processor. So, I'll be glad, if it gets accepted. Regarding tests, can you please point me in right direction, how to test it?

vstoykov commented 5 years ago

Regarding tests, can you please point me in right direction, how to test it?

Good question. I don't have the right answer.

When you are looking at pilkit/tests/test_processors.py is there something that can help you decide how to test it? I'm not sure if GaussianBlur is deterministic function or not and also is it deterministic between different PIL/Pillow versions. If it is then I think that it will not be hard to test that specific image processed with that processor is returning something predefined.

kravemir commented 5 years ago

When you are looking at pilkit/tests/test_processors.py is there something that can help you decide how to test it? I'm not sure if GaussianBlur is deterministic function or not and also is it deterministic between different PIL/Pillow versions. If it is then I think that it will not be hard to test that specific image processed with that processor is returning something predefined.

@vstoykov I'm not sure either, and I don't want to grow repository with image files. So, I've created tests to check, that:

Practically, the test is longer, than the implementation :-)

vstoykov commented 5 years ago

Thank you for your work.

When I look the tests I'm not sure that mocking in this case is the better approach. You don't need to grow the repo in size with extra images. There is pilkit/tests/assets/ where there are already two images which can be used in tests. In this case checking some particular pixel values after the processor I think that can be enough. What you think?

kravemir commented 5 years ago

When I look the tests I'm not sure that mocking in this case is the better approach. You don't need to grow the repo in size with extra images. There is pilkit/tests/assets/ where there are already two images which can be used in tests.

@vstoykov thanks for info. I'll use those images as tests inputs.

In this case checking some particular pixel values after the processor I think that can be enough. What you think?

I can also check 32x32px crop of the image center. And, commit tiny PNG to the repo. Based on Megapixel calculator, the size of a resulting PNG is estimated to be 595 B. So,.. it wouldn't grow repository heavily. Does that seem fine?

vstoykov commented 5 years ago

Checking for the crop is fine. There will be more than one pixel which will be more accurate.

vstoykov commented 5 years ago

After you change the tests you can rebase because there are conflicts with your branch that I merged already.

kravemir commented 5 years ago

@vstoykov I have rebased the branch, and changed tests to check outcome of GaussianBlur processor with two different radiuses, instead of mock calls, as it was before. Please, take a look at the PR.

vstoykov commented 5 years ago

It's merged by ac3877c1267ba0f1866f5eb843df079d7a1ea420