Add zscale interval based on Numdisplay's implementation #4776

saimn · 2016-04-12T21:29:44Z

Ref #4773
Currently this PR just add the zscale module from Numdisplay (BSD licensed), and provide an "interval" wrapper. Then a few questions before going on:

The zscale implementation works only for 2D images. It seems easy to adapt, but then there may other way to improve / speedup this code. Do we want to stick with the original module, or just use it as a basis and modify it if it is useful ? As Numdisplay is not really actively developed, I think the second option is fine, but feedback is welcome before spending more time on this ;)
What is the good way to mention the license of this external code ? Should I add a licenses/NUMDISPLAY_LICENSE.rst ?

pllim · 2016-04-13T12:35:01Z

👍 to modifying.

As for license, it is the same as PyFITS license, since it also came out of STScI, which is bound to AURA.

pllim · 2016-04-13T12:36:16Z

astropy/visualization/zscale.py

+KREJ = 2.5
+MAX_ITERATIONS = 5
+
+def zscale (image, nsamples=1000, contrast=0.25, bpmask=None, zmask=None):


Unless you can find a way to use bpmask and zmask, we can just remove them since they are not used (as stated in the docstring). I'll go through this in more detail when I get a chance.

pllim · 2016-04-13T12:37:20Z

c/c @larrybradley in case you are interested.

pllim · 2016-04-13T13:34:40Z

Also, @parejkoj mentioned this implementation by @ctslater.

pllim · 2016-04-13T14:46:37Z

astropy/visualization/tests/test_interval.py

+        interval = ZScaleInterval()
+        vmin, vmax = interval.get_limits(self.data)
+        np.testing.assert_allclose(vmin, -20.)
+        np.testing.assert_allclose(vmax, +60.)


Is the + necessary?

saimn · 2016-04-13T15:01:04Z

@pllim : Currently the zscale module is just a copy/paste from Numdisplay, I just added the docstring with the license. Agreed there is a lot of things to change in it to make it follow Astropy's coding rules, but that's was my point before starting hacking it ;).

Also the implementation from https://github.com/parejkoj/astropy/blob/luptonRGB/astropy/visualization/lupton_rgb.py#L55 already looks much simpler, so we may probably want to use that instead ? How should we proceed for this, @parejkoj @ctslater ? If you want to submit this as a new PR, I can close this one.

pllim · 2016-04-13T15:02:55Z

Yeah, the other one looks simpler, but are they interchangable? I am neutral either way.

If you do want to press ahead with this one and worry about performance, I would suggest profiling it first before attempting to optimize it.

saimn · 2016-04-13T22:50:04Z

Ok, I went through the original code and simplified as much as I can. The result is much simpler and easier to understand. It is more similar to the implementation from https://github.com/parejkoj/astropy/blob/luptonRGB/astropy/visualization/lupton_rgb.py#L55, except that it iteratively fits and masks the pixels with extreme values.
I think it also address most of your remarks @pllim. We still have 4 global variables, and I think if we could add these as optional parameters to zscale.zscale (and make this function public, importing it in astropy.visualization, as a complement to ZScaleInterval).

astrofrog · 2016-04-14T03:24:38Z

@saimn - thanks for working on this! Note that it will need an entry in the changelog for 1.2

pllim · 2016-04-14T13:38:41Z

Travis CI failure mentioned something wrong with np.polyfit call. Also if it stills complains about coverage, mark the untested blocks with # pragma: no cover to exclude them from coverage stats.

Sphinx also issued warnings but perhaps a rebase against master would fix those? Not sure.

pllim · 2016-04-14T13:39:28Z

When, this is all finalized, it is desirable to squash all the intermediate commits into one before merging.

bsipocz · 2016-04-14T13:40:40Z

The sphinx build only needs a restart on travis from someone who can.

saimn · 2016-04-15T12:30:19Z

Oh, it seems np.polyfit doesn't have the weight keyword in Numpy 1.6 ... Is it worth considering dropping support for this version ? ;-) (Numpy 1.11 was released recently, so 1.6 is quite old). Otherwise can I just throw an error for Numpy 1.6 in the zscale module ?

About the coverage: it seems the latest build failed for some network reason. The previous build seems fine (https://coveralls.io/builds/5778482) but I am always confused by coveralls' ui: it always display unrelated files, and I cannot find how to see the coverage by line (https://coveralls.io/builds/5778482/source?filename=astropy%2Fvisualization%2Finterval.py is empty, the new zscale module does not appear ...). @pllim Could you tell me how you saw the coverage ?

FTR, its gives exactly the same results.

+ Merge the zsc_fit_line function in the zscale

saimn · 2016-05-04T11:30:34Z

Rebased on master (and #4784) to get rid of Numpy 1.6 tests !

pllim · 2016-05-04T13:13:42Z

@saimn , how about adding a mention of this in the documentation?

saimn · 2016-05-04T21:35:28Z

@pllim - Good point, added it.

pllim · 2016-05-05T15:24:40Z

docs/visualization/normalization.rst

-``clip`` argument is provided to control the behavior of the normalization when
-values fall outside the limits::
+:class:`~astropy.visualization.PercentileInterval`,
+:class:`~astropy.visualization.AsymmetricPercentileInterval` and


Nitpicking: Add an Oxford comma.

@pllim - Done, didn't know this Oxford comma :)

astrofrog · 2016-05-10T07:44:30Z

@saimn - this looks great, and I can go ahead and merge it, but first, could you change:

# Licensed under a 3-clause BSD style license - see NUMDISPLAY_LICENSE.rst

to

# Licensed under a 3-clause BSD style license - see LICENSE.rst

That is, we are actually relicensing this file (and in the docstring below you can add a mention of NUMDISPLAY_LICENSE.rst if you like)

saimn · 2016-05-10T09:36:32Z

@astrofrog - ok, done !

astrofrog · 2016-05-10T10:07:35Z

@saimn - one quick question: how does this deal with NaN values?

saimn · 2016-05-10T21:43:40Z

@astrofrog - Oh, good point, and it doesn't deal with ... I have added a commit to filter the invalid values.

bsipocz · 2016-05-10T21:48:36Z

@saimn - since it was a regression, maybe it's worth adding a test with NaNs.

saimn · 2016-05-10T21:51:23Z

@bsipocz - yes indeed, seems reasonable, I will add a NaN in the test case.

astrofrog · 2016-05-11T13:01:22Z

This looks good - thanks @saimn!

saimn · 2016-05-11T13:03:36Z

Thanks :)

pllim reviewed Apr 13, 2016
View reviewed changes

pllim added Enhancement visualization labels Apr 13, 2016

pllim reviewed Apr 13, 2016
View reviewed changes

astrofrog added this to the v1.2.0 milestone Apr 14, 2016

saimn added 7 commits May 4, 2016 11:07

Simplify zscale by using common Numpy methods (median, std, ...)

3e7c64e

Use boolean mask to simplify bad pixels masking

15b911e

Use np.polyfit instead of custom line fitting.

a137ea2

FTR, its gives exactly the same results.

Update docstrings

61e9dc8

Add changelog entry

13f8095

Replace global variables with optional parameters.

216c9d1

+ Merge the zsc_fit_line function in the zscale

Add all parameters of the zscale function in the interval class

22258a4

saimn force-pushed the zscale branch from fb58b97 to 22258a4 Compare May 4, 2016 09:08

pllim reviewed May 5, 2016
View reviewed changes

Mention ZScaleInterval in the docs

b49236e

saimn force-pushed the zscale branch from 21fe5ec to b49236e Compare May 5, 2016 20:31

Adapt license mention [ci skip]

2b3699a

astrofrog added the Ready-for-final-review label May 10, 2016

Remove invalid values before sampling

63e5d4f

saimn force-pushed the zscale branch from 360442e to 63e5d4f Compare May 10, 2016 21:58

astrofrog merged commit d001500 into astropy:master May 11, 2016

saimn deleted the zscale branch May 11, 2016 13:08

saimn mentioned this pull request Nov 28, 2016

Is zscale.zscale private #5499

Closed

Uh oh!

Add zscale interval based on Numdisplay's implementation #4776

Add zscale interval based on Numdisplay's implementation #4776

Uh oh!

Conversation

saimn commented Apr 12, 2016

Uh oh!

pllim commented Apr 13, 2016

Uh oh!

pllim Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

pllim commented Apr 13, 2016

Uh oh!

pllim commented Apr 13, 2016

Uh oh!

pllim Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

saimn commented Apr 13, 2016

Uh oh!

pllim commented Apr 13, 2016

Uh oh!

saimn commented Apr 13, 2016

Uh oh!

astrofrog commented Apr 14, 2016

Uh oh!

pllim commented Apr 14, 2016

Uh oh!

pllim commented Apr 14, 2016

Uh oh!

bsipocz commented Apr 14, 2016

Uh oh!

saimn commented Apr 15, 2016

Uh oh!

saimn commented May 4, 2016

Uh oh!

pllim commented May 4, 2016

Uh oh!

saimn commented May 4, 2016

Uh oh!

pllim May 5, 2016

Choose a reason for hiding this comment

Uh oh!

saimn May 5, 2016

Choose a reason for hiding this comment

Uh oh!

astrofrog commented May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented May 10, 2016

Uh oh!

astrofrog commented May 10, 2016

Uh oh!

saimn commented May 10, 2016

Uh oh!

bsipocz commented May 10, 2016

Uh oh!

saimn commented May 10, 2016

Uh oh!

astrofrog commented May 11, 2016

Uh oh!

saimn commented May 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

astrofrog commented May 10, 2016 •

edited

Loading