-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Notebook updates #131
Conversation
…parency artifacts
…ed more spreading examples.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
This PR is still somewhat WIP, but it's ready for the individual commits to reviewed:
Allows cmap=reversed(Greys9), instead of the awkward cmap=list(reversed(Greys9))
I think a square is less visually distracting, but could be persuaded otherwise.
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.
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.
Incorporated Peter's edits and clarified why downsampling is dangerous for time series.
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.
Minor updates to bring notebooks up to date.