Refactor image plot #147
Refactor image plot #147
Conversation
|
@itziakos This PR is likely to be relevant for the project you are working on, it might be worth testing it. |
| # Properties | ||
| #------------------------------------------------------------------------ | ||
|
|
||
| @property |
There was a problem hiding this comment.
should these be Trait Properties?
|
Thanks @pberkes, I had quick look and left a few comments. I do not have however bandwidth to check if it affects the client project until probably late next week. I will see is others might be able to look at it. |
|
I can probably get to this before @itziakos. I'll try to take a look soon. |
| if len(data.shape) != 3: | ||
| raise RuntimeError("`ImagePlot` requires color images.") | ||
| elif data.shape[2] not in KIVA_DEPTH_MAP: | ||
| msg = "Unknown colormap depth value: {}" |
There was a problem hiding this comment.
If you want to support python 2.6, use {0} rather than a bare {}.
|
Aside from some minor comments above, 👍. |
| """ | ||
| # Upper-right values are always larger than lower-left values, | ||
| # regardless of origin or orientation... | ||
| (lower_left, upper_right) = self.index.get_bounds() |
There was a problem hiding this comment.
Note that in current implementation of GridDataSource (which is the default index for ImagePlots) you need to be careful about what you provide as the x and y arrays: they need to be the coordinates of the edges of the mapped pixels of the image, and so you need one more value in the index arrays than the image has pixels.
This sucks, and should probably be fixed somehow.
But right now it means that you have a problem with the tests.
|
Warren went over this code a few years ago, if I remember, because the original version wasn't rendering correctly when extremely zoomed in (eg. with only part of one-or-two pixels showing in the viewable area, possibly on just one of the directions). You should make sure your code works in these situations as well. |
As suggested by @corranwebster
|
@corranwebster I don't see any issues with extreme zoom, but I can't even reproduce the issue on master if I remove the block of code in [1] https://github.com/enthought/chaco/blob/master/chaco/image_plot.py#L279 |
|
@tonysyu Fair enough, although looking at the code, it seems as though its intent is to provide an effective maximum to the scaling factor, so it may have been an efficiency hack. The case that we were dealing with was a situation where we had gigabytes of sensor data and users were zooming in to very small regions inside the data. You might see if you can get @vkalatsky or @jwiggins to try this branch on the project which I am not giving the name of, but which you can probably guess that I am referring to (or we can discuss in person later). |
|
@corranwebster It turns out I was wrong that the extreme zoom levels weren't a noticeable issue. Zooming into the image in I've also incorporated changes in #199 to fix #198. @tangobravo Are you able to confirm that this branch resolves the issue you were seeing? |
|
@tonysyu Yes, I have just tested with your branch and that resolves my issue too. I've moved away from chaco for my project though - there was an annoying redrawing bug when stretch_data was set False with the plot origin as 'top left' - something goes wrong with the layout so the plot disappears on every other resize message. I decided I was going to be spending too much time fixing chaco bugs (I'd already got much deeper into things that I wanted too for #198) and not enough time on my actual problem. |
|
@tangobravo Sorry your experiences with chaco have been so poor. If you don't mind another follow up: When you say "resize" do you mean zoom? If so, maybe this is the same as #144. If instead you mean resizing the figure window, I can't seem reproduce the issue, unfortunately. |
|
@corranwebster @cfarrow @ALL @rkern Any more comments on this or is this ready to merge? |
|
@tonysyu Do you mind resolving the conflict. You are touching more lines than I feel comfortable resolving... |
Conflicts: chaco/image_plot.py Hopefully, the changes from PR #219 were integrated correctly.
|
Bump @cfarrow (or @corranwebster, if you're looking for something to review ;) |
|
@tonysyu Could you add a brief summary of the PR to the |
|
Having been in the Chaco codebase for a bit the last week or two, I think some of the issues Chris was seeing may be due to this: https://github.com/enthought/chaco/blame/master/chaco/plot_graphics_context.py#L14 It seems that sometimes Chaco wants to play games with shifting half pixels compared to Kiva. That comment claims Kiva is wrong, but I don't know if that is in fact the case (and I don't know who the author of the comment is): my expectation is that the coordinate system should have integer values for the edges of the pixels (this is pretty standard, see the HTML Canvas element, for example) and half-integer values for the center of pixels. In any case, this PR seems fine apart from the minor comment about updating the changelog. |
|
@corranwebster: Thanks for reviewing! Changelog comment added. |
|
Changes Unknown when pulling 93e131f on refactor/image-plot into * on master*. |
|
🎉 |
|
⛳ 👏 |
While adding some plotting capabilities to an
ImagePlotsubclass (see https://github.com/enthought/geolibs/pull/43), I ended up having to refactorImagePlotquite a bit.This PR cleans up some of the coordinate transforms in
ImagePlotand fixes an issue with negative screen sizes (see enthought/enable#109). I've also added tests, but I'm not sure how robust this testing is: It requires precise pixel positions in the rendered image. (This positioning is achieved using a tweaked index grid, tweaked plot bounds, and slicing some "extra" rows/columns off the rendered image. Probably a bit fragile :P)