Skip to content
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

Notebook updates #131

Merged
merged 16 commits into from
Mar 31, 2016
Merged

Notebook updates #131

merged 16 commits into from
Mar 31, 2016

Conversation

jbednar
Copy link
Member

@jbednar jbednar commented Mar 30, 2016

This PR is still somewhat WIP, but it's ready for the individual commits to reviewed:

648926c Allowed cmap to accept a reversed() list

Allows cmap=reversed(Greys9), instead of the awkward cmap=list(reversed(Greys9))

c6162a2 Changed smallest circle shape for spreading from a + to a square

I think a square is less visually distracting, but could be persuaded otherwise.

d2e3d9b Changed default bin number for eq_hist improve accuracy of float normalization

eq_hist ignores the bin number for integers, which is what we usually use it for, but for floats it's crucial to have enough bins. 256 is clearly not enough, but I'm not sure how high to go.

4611376 Added spreading support for Pipeline, now defaulting to dynspread

A Pipeline is meant as a simple object that covers most cases, and in e.g. the non-geo example we definitely need spreading by default, since the datapoints are all very far from each other.

a55302f Expanded and updated timeseries notebook

Incorporated Peter's edits and clarified why downsampling is dangerous for time series.

0a640e9 Changed default interpolate and colorize method to eq_hist

The two defensible choices for the default interpolate method are eq_hist and linear. eq_hist seems safer since it reveals structure that one might not realize was there otherwise.

8305880 Minor census cleanup
85792d9 Updated non-geo example to use eq_hist

Minor updates to bring notebooks up to date.

@jbednar
Copy link
Member Author

jbednar commented Mar 30, 2016

Ok, I think this is ready to merge, though it would be good if some one else could look at it, as there are a few controversial bits that involve changes to the library itself (isolated into specific commits as listed above).

@@ -245,7 +247,7 @@ def set_background(img, color=None):
if color is None:
return img
background = np.uint8(rgb(color) + (255,)).view('uint32')[0]
data = source(img.data, background)
data = over(img.data, background)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change this? This doesn't set the background, it overlays an image over a background of that color. For images that use alpha to indicate magnitude (output of colorize), this will make all set pixels have full alpha. I'm unsure if this is desired here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, and intended. The problem was that previously, if one tried to set the background to black, it only changed the fully transparent pixels, which had very strange results -- if you take such an image and view it in e.g. Preview on a Mac, it's all garbled: the fully transparent pixels are black, but Preview's default gray background shines through (to a greater or lesser extent, depending on alpha) all the others. So I don't think that set_background was doing something useful before; changing only the fully transparent pixels while removing their transparency yet leaving other pixels transparent doesn't result in a usable image in any scenario I can think of, and certainly not in the use cases I had in mind for set_background.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second look, I think I see what you're saying here, but I don't think it's true. I.e., you're worried that the alpha channel will simply be discarded from the top image? The code isn't discarding the src alpha, as far as I can see; it's using it to control how the src gets mixed with the background, which is what we want here. Try comparing census.ipynb with set_background using over and source for comparison, and you should see what I mean...

@jbednar jbednar merged commit 4224702 into master Mar 31, 2016
@jbednar jbednar deleted the nb-updates branch March 31, 2016 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants