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

Do not propagate dataset property through projection operations #414

Closed
wants to merge 3 commits into from

Conversation

jonmmease
Copy link

This PR addresses an issue I ran into while trying to create a version of the glaciers demo using the HoloViews link_selections function.

In this demo, the kdims of a Dataset are projected using the GeoViews project_points operation. This operation returns a new Dataset with kdims that have the same names, but their values are transformed into a new coordinate system. Without this PR, the .dataset property of the resulting object will be inherited from the parent object. This breaks link_selections because you end up with fields in obj.dataset that have the same names as fields in obj, but their values are in different coordinate systems.

Along with holoviz/holoviews#4137, This PR simply disables dataset propagation through projection operations.

An alternative to disabling propagation would be to require that projections introduce new field names, but I expect that this would be a significantly breaking change

This prevents HoloViews from automatically propagating the .dataset and .pipeline
properties through an operation.  We don't want this propagation to happen for projections
because they change the values of the dataset key columns while keeping the same names.
@philippjfr
Copy link
Member

An alternative to disabling propagation would be to require that projections introduce new field names, but I expect that this would be a significantly breaking change

I think in the longer term we could make use of the Dimension.unit field to make these distinctions but for now this seems reasonable.

@philippjfr
Copy link
Member

Hmm, can't really work out what's up with the failing tests yet.

@jonmmease
Copy link
Author

Hmm, can't really work out what's up with the failing tests yet.

Oh, right. These tests that were updated will need holoviz/holoviews#4137 released in order to pass.

@philippjfr
Copy link
Member

I'll try to get this merged, but do you think we could have some mechanism to register transforms which convert the coordinates back to the original coordinate system?

@jonmmease
Copy link
Author

do you think we could have some mechanism to register transforms which convert the coordinates back to the original coordinate system?

From the perspective of link_selections, we would need a way to invert the transformation and somehow run the selection expression through that inverse operation so that the inverse-transformed expression can be meaningfully applied to the element's .dataset object.

I'm thinking more and more that we may need to make a selection a class that can produce a dim-expression on demand. That way the concept of a coordinate transform could be handled by the class, rather than trying to perform the transform on the dim expression.

@jonmmease
Copy link
Author

I think we can close this @philippjfr, though I don't exactly remember the outcome. Did you do some work in HoloViews itself to handle projections in the pipeline?

@philippjfr
Copy link
Member

Still need to revisit this. I'll leave it open for now.

@jonmmease
Copy link
Author

Closing to clean up my PR history, but feel free to re-open if someone wants to pick this up again.

@jonmmease jonmmease closed this Sep 6, 2023
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