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

Implement Image interface #994

Merged
merged 70 commits into from
Apr 8, 2017
Merged

Implement Image interface #994

merged 70 commits into from
Apr 8, 2017

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Nov 30, 2016

This PR is a WIP in implementing an interface to allow Image Elements to subclass Dataset and use the other interfaces, replacing GridImage. There are a few things to be settled so I'm hoping to discuss these in this PR.

  1. Our Image types need bounds. When you pass in an array the bounds are required, but when using the xarray and grid interfaces, the bounds should be computed from the data. Currently I deduce the bounds by asserting that all samples must be evenly spaced with a small tolerance (10e-9). Eventually we'll have QuadMesh for non-equal spacing, but that will require some more work. Does this sound reasonable?

  2. Dataset does not have an interface to access multiple value arrays at once. This makes it difficult to access RGB and HSV data. We can either accept the overhead of stacking and unstacking these arrays and provide a convenient property on RGB/HSV elements to do this for us, or come up with a general API on the interfaces to get the stacked arrays. There was also the suggestion that other interfaces could support holding stacked value dimension arrays instead of holding them as unstacked arrays per value dimension but that could happen in a later PR.

@jlstevens
Copy link
Contributor

Just to summarize a discussion I had with Philipp on gitter regarding the first question:

Our Image types need bounds. When you pass in an array the bounds are required, but when using the xarray and grid interfaces, the bounds should be computed from the data. Currently I deduce the bounds by asserting that all samples must be evenly spaced with a small tolerance (10e-9). Eventually we'll have QuadMesh for non-equal spacing, but that will require some more work. Does this sound reasonable?

We agreed that this is a reasonable approach as long as the tolerance is about avoiding machine precision issues only and not some fuzzy ad hoc tolerance to be tweaked by the user. For instance, instead of 10e-9 a suitable value should be determined for the system, e.g using the sys.float_info.

@jlstevens
Copy link
Contributor

Again summarizing our discussion on gitter:

Dataset does not have an interface to access multiple value arrays at once. This makes it difficult to access RGB and HSV data. We can either accept the overhead of stacking and unstacking these arrays and provide a convenient property on RGB/HSV elements to do this for us, or come up with a general API on the interfaces to get the stacked arrays. There was also the suggestion that other interfaces could support holding stacked value dimension arrays instead of holding them as unstacked arrays per value dimension but that could happen in a later PR.

I agree that a future PR supporting multiple vdims in the data API would be very welcome but for now, we can extend dimension_values on RGB elements only. Specifically, we can adapt RGB to make RGB.dimension_values(['R', 'G', 'B']) work in this PR and in a later PR we can support stacked output from dimension_values (for lists of vdims only).

@philippjfr
Copy link
Member Author

philippjfr commented Jan 22, 2017

Now based off #1075 and now slowly reaching API compatibility. Still missing a sample implementation otherwise largely complete. Should also add a lot more unit tests for grid interfaces, which will also cover the new ImageInterface.

Currently I'm also still a bit unclear what to do about Raster itself. It could probably be unified with Image by simply adding integer indices in the constructor, but that would also make Raster inherit from SheetCoordinateSystem.

@philippjfr
Copy link
Member Author

Raster is now unified with the image interface. Now I'll have to refactor QuadMesh so it still works independently. Hopefully we'll manage to write interfaces to handle 1D and 2D binned data, e.g. for Histogram and QuadMesh, as part of 1.8.


def test_raster_ellipsis_slice_value_missing(self):
data = np.random.rand(10,10)
try:
hv.Raster(data)[...,'Non-existent']
except Exception as e:
if str(e) != repr("'z' is the only selectable value dimension"):
print(e)
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't forget to remove.

@philippjfr philippjfr modified the milestones: v2.0, v1.7.0 Mar 15, 2017
@philippjfr philippjfr self-assigned this Mar 15, 2017
@philippjfr philippjfr force-pushed the image_interface branch 7 times, most recently from 7736826 to fa4e944 Compare March 27, 2017 16:47
@philippjfr
Copy link
Member Author

Ready for final review and merging once test pass.

@philippjfr
Copy link
Member Author

Nevermind still need to make a few changes.

@philippjfr
Copy link
Member Author

Ready to merge again, some issues reindexing gridded datasets cropped up but are fixed now.

"""
Computes a bounding range from a number of evenly spaced samples.
Will raise an error if samples are not evenly spaced within
tolerance.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just mention which tolerance - namely as reported by sys.float_info

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer raises an error, so yeah will fix it.

@@ -52,6 +43,54 @@ def compute_edges(edges):
return np.concatenate([edges, [edges[-1]+width]])


def compute_slice_bounds(slices, scs, shape):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have this code split out as a utility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I would recommend adding tests for this utility right now as I do want to merge ASAP. At least it is much easier to test now!

@jlstevens
Copy link
Contributor

Looks good. I made one comment about a docstring - other than that, I'll merge when you tell me it is ready (and the tests are green).

@philippjfr
Copy link
Member Author

Already pushed the updated docstring, just waiting on tests now.

@jlstevens
Copy link
Contributor

All tests are green! Merging!

@jlstevens jlstevens merged commit 6a1b38e into master Apr 8, 2017
@philippjfr philippjfr deleted the image_interface branch April 11, 2017 12:19
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants