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

Implemented redim method on Dimensioned objects #715

Merged
merged 3 commits into from
Jun 9, 2016
Merged

Implemented redim method on Dimensioned objects #715

merged 3 commits into from
Jun 9, 2016

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jun 8, 2016

This PR adds the redim method to easily replace dimensions or certain attributes on dimensions in a nested object (following the proposal in #714). The dimension overrides are supplied as keyword arguments to the method with the keyword matching the existing dimension by name and the value supplying one of three overrides:

  1. A simple string to rename the dimension, e.g. curve.redim(x='Time').
  2. A dictionary of parameter settings, e.g. curve.redim(x={'range': (0, 10), 'name': 'Time'})
  3. A new Dimension object, e.g. curve.redim(x=hv.Dimension('Time')).

The objects the redim applies to can be narrowed down with a specification, which is already in use on the traverse, map and select method. This allows only applying the redim to certain objects in three different ways:

  1. The type of the object, e.g. layout.redim(hv.Curve, x='Time')
  2. A string specification of the form type{{.group}.label}, e.g. layout.redim('Curve.GDP', x='Time')
  3. A function, e.g. layout.redim(lambda x: x.vdims[0].name == 'GDP', x='Time')

The implementation exists in two places, on Dimensioned and on Dataset. The implementation on Dataset is required because a change to the dimension names has to be reflected in the underlying datasource. The rename implementation on all the interfaces so far is pretty trivial and I think it's good functionality to have anyway. Overall I'm pretty happy with this and will be very useful to have, as now it'll be possible to quickly rename dimensions and adjust their ranges.

I still need to work on the docstrings, docs and add unit tests.

@@ -210,7 +215,7 @@ def add_dimension(cls, columns, dimension, dim_pos, values, vdim):
@classmethod
def dframe(cls, columns, dimensions):
if dimensions:
return columns.reindex(columns=dimensions)
Copy link
Member Author

Choose a reason for hiding this comment

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

This change snuck in and is unrelated, but I've been meaning to commit it for a while anyway.

@philippjfr
Copy link
Member Author

I've now added unit tests, so the PR is ready for review.

@jbednar
Copy link
Member

jbednar commented Jun 8, 2016

Looks good to me!

@jlstevens
Copy link
Contributor

Ok, it makes sense. It definitely has to be on Dimensioned and the reason you need some implementation on the interfaces is also clear.

My only question is why is it called rename on the interfaces too? Why not also call it redim? Maybe I'm missing something but I think it might be more consistent to have one name to consider denoting one concept. Even if the signature isn't identical etc, to me redim makes sense on an interface and well as on a dimensioned object.

Other than that one comment, I like this PR and am happy to merge.

@philippjfr
Copy link
Member Author

My only question is why is it called rename on the interfaces too? Why not also call it redim? Maybe I'm missing something but I think it might be more consistent to have one name to consider denoting one concept. Even if the signature isn't identical etc, to me redim makes sense on an interface and well as on a dimensioned object.

I think I agree with you and may even say that the signatures should be the same. Some interfaces like iris also supplying units so those should also be updated. I'll go ahead and make that change.

@philippjfr
Copy link
Member Author

Okay, my latest commit has renamed the interface methods from rename to redim for consistency. I think this is ready to merge now.

@philippjfr philippjfr force-pushed the redim branch 2 times, most recently from 703e2a8 to 4595067 Compare June 9, 2016 10:49
@jlstevens
Copy link
Contributor

Ah, the push build passes but the pr one didn't? Is it a transient error?

@philippjfr
Copy link
Member Author

Looks like it. Restarting.

@philippjfr
Copy link
Member Author

So this makes no sense, one of the new unit tests is failing but only the PR build. Both the push build and my local copy work just fine.

@philippjfr
Copy link
Member Author

Finally passing, turned out to be my fault. Part of the tests in this PR depended on the other iris PR.

@jlstevens
Copy link
Contributor

Ok. Looks good! Merging.

@jlstevens jlstevens merged commit 81b5601 into master Jun 9, 2016
@jlstevens jlstevens deleted the redim branch July 12, 2016 22:56
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.

None yet

3 participants