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 new image normalization classes #5206

Merged
merged 21 commits into from Sep 13, 2016

Conversation

Projects
None yet
4 participants
@larrybradley
Member

larrybradley commented Jul 28, 2016

This PR contains the following changes to the visualization subpackage:

  • Adds a Norm class to generate a matplotlib.colors.Normalize object from an interval and a stretch instance.
  • Adds a convenience ImgNorm class for quick interactive analysis (and used by the fits2bitmap command-line script). ImgNorm replaces the scale_image function.
  • Deprecates the scale_image function (in favor of the ImgNorm class).
  • Adds a default stretch for the existing Normalization class.
  • Adds default vmin/vmax for the ManualInterval class.
  • Adds missing API docs for existing interval and stretch classes.
  • Adds addition tests for the visualization subpackage.
  • Augments and updates the visualization narrative docs.

Examples

With the existing ImageNormalize class one needs to input the vmin/vmax values, which is a bit cumbersome, e.g.:

interval = MinMaxInterval()
vmin, vmax = interval.get_limits(image)
norm = ImageNormalize(vmin=vmin, vmax=vmax, stretch=SqrtStretch())

With the new Norm class inputs, one can simply do, e.g.:

norm = ImageNormalize(image, MinMaxInterval(), SqrtStretch())

And then the image is displayed with:

ax.imshow(image, origin='lower', norm=norm)

cc: @astrofrog

@pllim pllim added the visualization label Jul 28, 2016

@pllim pllim added this to the v1.3.0 milestone Jul 28, 2016

@astrofrog astrofrog self-assigned this Jul 30, 2016

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jul 30, 2016

@larrybradley - I'm in favor of having conveniences such as these, but I think that having Normalize, Norm, and ImgNorm is confusing - instead I would propose making the two new classes you are adding here class methods for Normalize. In fact, Norm could even be merged with the existing Normalize initializer so that you can either specify vmin and vmax or interval and image in __init__.

ImgNorm can then be made into a convenience class method (I'm trying to think of a good name).

"""
def __call__(self, values, out=None, clip=True):
def __call__(self, values, clip=True, out=None):

This comment has been minimized.

@astrofrog

astrofrog Jul 30, 2016

Member

Just out of curiosity, why the change?

This comment has been minimized.

@larrybradley

larrybradley Aug 1, 2016

Member

To make it consistent with the BaseInterval __call__ method. Also, the underlying numpy functions have out as the last keyword.

If you agree with out being last, then I should also change the calling sequence in the stretch._prepare function, so that everything is consistent.

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

shouldn't this be mentioned in the changelog?

This comment has been minimized.

@larrybradley

larrybradley Sep 9, 2016

Member

I suspect that not many people were actually calling stretch instances directly on the data (with or without the out keyword), but I'm happy to mention it in the changelog. @astrofrog?

@pllim

This comment has been minimized.

Member

pllim commented Aug 1, 2016

trying to think of a good name

ConvenientNorm? NormForLazyPeople? EasyNorm? QuickNorm?

@astrofrog

This comment has been minimized.

Member

astrofrog commented Aug 1, 2016

@pllim - just to be clear, I meant a class method not class factory (so Normalize.from_...)

@pllim

This comment has been minimized.

Member

pllim commented Aug 1, 2016

from_image?

@larrybradley

This comment has been minimized.

Member

larrybradley commented Aug 1, 2016

@astrofrog Indeed, I was struggling with short-ish names (because these are convenience classes) similar to ImageNormalize. Suggestions welcome!

@larrybradley

This comment has been minimized.

Member

larrybradley commented Aug 1, 2016

@astrofrog I assume you mean the ImageNormalize class (in astropy visualization), not the Normalize class (in matplotlib).

I don't think having these new classes as ImageNormalize methods is very convenient. It's also very confusing. Is something like this what you were thinking:

norm = ImageNormalize(stretch=SqrtStretch())
norm2 = norm.apply_interval(data, interval=MinMaxInterval())
imshow(data, norm=norm2)

I didn't want to change ImageNormalize, but it could be combined with Norm. I think that is much better and less confusing than adding methods (and it requires one less unique class name 😄). It would require a bit of input validation checking to make sure the right combination of keywords are inputs. Sound OK?

We'd still need a new name for ImgNorm. This class essentially replaces the scale_image function, but scale can mean so many things, so it's not the best name.

@astrofrog

This comment has been minimized.

Member

astrofrog commented Aug 1, 2016

I didn't want to change ImageNormalize, but it could be combined with Norm. I think that is much better and less confusing than adding methods (and it requires one less unique class name 😄). It would require a bit of input validation checking to make sure the right combination of keywords are inputs. Sound OK?

Yep!

@larrybradley larrybradley force-pushed the larrybradley:img_norm branch from 1502945 to 300cd63 Aug 10, 2016

@larrybradley

This comment has been minimized.

Member

larrybradley commented Aug 10, 2016

@astrofrog I've implemented the new changes:

  • ImageNormalize can be initialized with either data/interval or vmin/vmax (which takes precedence).
  • Replaced scale_image (deprecated) with a new function called mpl_norm that returns an ImageNormalize object. (The ImgNorm class from the first implementation of this PR is gone).

@larrybradley larrybradley force-pushed the larrybradley:img_norm branch 2 times, most recently from d5dbc5f to ad4ab4a Aug 11, 2016

CHANGES.rst Outdated
@@ -29,9 +29,6 @@ New Features
- ``astropy.modeling``
- Add a class to combine astropy fitters and functions to remove outliers
e. g., sigma clip. [#4760]

This comment has been minimized.

@astrofrog

astrofrog Aug 12, 2016

Member

Is there a reason why you are removing this?

This comment has been minimized.

@larrybradley

larrybradley Aug 12, 2016

Member

Not sure how that happened. Thanks for catching it. It's fixed.

CHANGES.rst Outdated
deprecated, since this class does not allow stacks of matrices. As a
result, the semi-public functions ``angles.rotation_matrix`` and
``angles.angle_axis`` are also deprecated, in favour of the new routines
with the same name in ``coordinates.matrix_utilities``. [#5104]

This comment has been minimized.

@astrofrog

astrofrog Aug 12, 2016

Member

Same here - looks like an issue with conflict resolution?

This comment has been minimized.

@larrybradley
@astrofrog

This comment has been minimized.

Member

astrofrog commented Aug 12, 2016

@larrybradley - thanks for your work on this. I think simple_image_norm would be better than mpl_norm - changing mpl to image because it returns an ImageNormalization class, and adding simple to make it clearer that this is a convenience function that only supports a subset of cases.

@larrybradley

This comment has been minimized.

Member

larrybradley commented Aug 12, 2016

@astrofrog Would image_norm be acceptable instead? I understand the reasoning behind prepending simple, but that is explained in the docs.

@larrybradley larrybradley force-pushed the larrybradley:img_norm branch from ad4ab4a to 5dd14bb Aug 12, 2016

@larrybradley

This comment has been minimized.

Member

larrybradley commented Sep 9, 2016

@astrofrog I renamed the function simple_norm.

@astrofrog

This comment has been minimized.

Member

astrofrog commented Sep 9, 2016

This all looks good to me now - thanks! Is this good to go from your side?

@larrybradley

This comment has been minimized.

Member

larrybradley commented Sep 9, 2016

Yep, it's good to go.

@@ -6,39 +6,73 @@
"""
from __future__ import division, print_function

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

why remove the blank line?

I always thought they were meant to seperate builtins, externals and internal packages

This comment has been minimized.

@larrybradley

larrybradley Sep 9, 2016

Member

I'm not a fan of the blank lines within the imports. I wasn't aware that was recommended.

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

I'm not sure if it's really recommended. PEP8 states they should be grouped and I always interpreted this as blank lines. 😄

edit: Ok, they even mention the blank line.

This comment has been minimized.

@larrybradley

larrybradley Sep 9, 2016

Member

I do order them, just without blank lines. :) Ah, OK. pep8 has failed me all these years (it could at least check the standard library imports).

This comment has been minimized.

@pllim

pllim Sep 9, 2016

Member

Blank line also makes it stands out, so easier to remove when no longer needed.

This comment has been minimized.

@larrybradley

larrybradley Sep 9, 2016

Member

@pllim But the __future__ import always goes first, so it's hard to miss! 😄

vmin, vmax = self.get_limits(values)
if out is None:
values = np.subtract(values, float(vmin))
else:
if out.dtype.kind != 'f':
raise TypeError("Can only do in-place scaling for floating-point arrays")
raise TypeError('Can only do in-place scaling for '

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

Somwhere it was mentioned that error messages should start with a lowercase. Don't remember where though.

This comment has been minimized.

@larrybradley

larrybradley Sep 9, 2016

Member

I always write them as sentences (what if the message contains multiple sentences?). In any case, @astrofrog actually wrote this one, I just wrapped the line.

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

At least the python-builtin exceptions start lowercase. I think the rational was that the sentence starts with the capitalized exception name and then the first sentence naturally would be lowercase.

I don't mind the capitalized start so feel free to ignore this.

"""
from __future__ import division, print_function

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

same question here regarding the line removal.

try:
import matplotlib # pylint: disable=W0611
import matplotlib # pylint: disable=W0611

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

why increase the number of whitespaces? two are recommended before inline comments.

This comment has been minimized.

@larrybradley

larrybradley Sep 9, 2016

Member

I think it's at least two spaces. I think this was inadvertent, so I could change it back.

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

You are right it's at least 2 spaces.

@@ -6,20 +6,19 @@
"""
from __future__ import division, print_function

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

same question here regarding the blank lines.

from the array (with replacement)
Maximum number of values to use. If this is specified, and there
are more values in the dataset as this, then values are randomly
sampled from the array (with replacement)

This comment has been minimized.

@MSeifert04

MSeifert04 Sep 9, 2016

Contributor

Missing dot at the end of the sentence.

@MSeifert04

This comment has been minimized.

Contributor

MSeifert04 commented Sep 9, 2016

I'm sorry for the flood of stylistic comments. I don't feel strong about them, so feel free to ignore these.

@larrybradley larrybradley force-pushed the larrybradley:img_norm branch from fab96d9 to 6a5a65d Sep 9, 2016

@larrybradley

This comment has been minimized.

Member

larrybradley commented Sep 9, 2016

Thanks, @MSeifert04. I've implemented everything (except explaining the hist-eq values default).

@astrofrog Now this should be ready to go.

@larrybradley larrybradley force-pushed the larrybradley:img_norm branch from 6a5a65d to b4ef45f Sep 10, 2016

@astrofrog astrofrog merged commit 3ffe796 into astropy:master Sep 13, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 77.251%
Details
@astrofrog

This comment has been minimized.

Member

astrofrog commented Sep 13, 2016

Thanks @larrybradley and sorry for the delay in merging!

@larrybradley larrybradley deleted the larrybradley:img_norm branch Sep 20, 2016

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