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 Image.normalize() support #95

Merged
merged 9 commits into from Mar 4, 2013

Conversation

2 participants
@inactivist
Contributor

inactivist commented Mar 3, 2013

My first attempt at adding functionality to Wand. This pull request adds support for the MagickNormalizeImage* methods.

Edit: I've provided unit tests that normalize a grayscale gradient image (assets/gray_range.jpg) on all channels (the default normalize() operation) and the 'red' channel and then checks for expected color values at the four corners.

Comments welcome.

:param channel: The channel to normalize. Defaults to 'all_channels'
:type channel: integer (one of the CHANNELS dict values.)
"""
r = library.MagickNormalizeImage(self.wand, channel)

This comment has been minimized.

@dahlia

dahlia Mar 3, 2013

Collaborator

Shouldn’t this call MagickNormalizeImageChannel(MagickWand *wand, const ChannelType channel) or remove second argument to satisfy the interface of MagickNormalizeImage(MagickWand *wand)?

http://www.imagemagick.org/api/magick-image.php#MagickNormalizeImage

It would be better if the default value of channel is None and it conditionally calls MagickNormalizeImageChannel() or MagickNormalizeImage().

This comment has been minimized.

@inactivist

inactivist Mar 3, 2013

Contributor

Good points. I'll rewrite and resubmit.

@dahlia

This comment has been minimized.

Collaborator

dahlia commented Mar 3, 2013

Thanks for your effort. 😄 Could you write unit tests for this as well?

@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 3, 2013

Yes, I will see what I can do about unit tests. I'll take a look at some existing tests.

Fix Image.normalize() channel arg and related handling.
Image.normalize() channel arg now defaults to None, and
calls library.MagickNormalizeImageChannel() if != None
@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 3, 2013

Next up: unit tests...!

which will normalize all channels.
:type channel: integer (one of the CHANNELS dict values.)
"""

This comment has been minimized.

@dahlia

dahlia Mar 3, 2013

Collaborator

The channel parameter should take a string instead of integer. See channel parameter of Image.composite_channel() method and CHANNELS constant mapping.

This comment has been minimized.

@inactivist

inactivist Mar 4, 2013

Contributor

Can you tell I'm new around here? (ImageMagick and Wand...)
Thanks for the guidance.

This comment has been minimized.

@dahlia

dahlia Mar 4, 2013

Collaborator

It might help.

@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 4, 2013

As far as testing goes, I'm looking for good sample images to use in testing channel functionality. Recommendations?

@dahlia

This comment has been minimized.

Collaborator

dahlia commented Mar 4, 2013

How about sample images used by this manual?

@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 4, 2013

I saw those examples, they don't show usage for separate channels (such as red, green, blue) as far as I can see. I'm still looking for examples using the convert command with -normalize, -channel operating on single R,G,B channels...

@dahlia

This comment has been minimized.

Collaborator

dahlia commented Mar 4, 2013

@inactivist It seems almost done, do you have any plans to work on this more?

@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 4, 2013

@dahlia Yes - I'd like to come up with a simple unit test for per-channel normalize functionality as that is not currently being tested.

@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 4, 2013

@dahlia I think I'm done for now unless you can suggest other improvements. (I'm sure the unit tests could be more elegant but they do seem to work as-is.)

dahlia added a commit that referenced this pull request Mar 4, 2013

Merge pull request #95 from inactivist/add-normalize
Add Image.normalize() support

@dahlia dahlia merged commit dfc193e into emcconville:master Mar 4, 2013

1 check passed

default The Travis build passed
Details
@inactivist

This comment has been minimized.

Contributor

inactivist commented Mar 4, 2013

Thanks! 👍

@inactivist inactivist deleted the inactivist:add-normalize branch Mar 4, 2013

dahlia added a commit that referenced this pull request Mar 4, 2013

dahlia added a commit that referenced this pull request Mar 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment