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

Unified redim implementation with convenience methods #1302

Merged
merged 7 commits into from Apr 17, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Member

jlstevens commented Apr 17, 2017

This PR unifies redim and makes it much more convenient to use, which is especially helpful for DynamicMap declarations. You can now do:

image

Instead of the more verbose equivalent we have been using till now:

image

Even for this simplest possible example with only a single kdim, the new style is shorter.

All existing use of redim should act in exactly the same way. What is new are all the redim auxiliary methods that make it easier to pick a particular Dimension parameter to change.

Tasks

  • Unify redim, removing existing redim methods
  • Implement auxiliary redim methods
  • Write more redim unit test
@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 17, 2017

I've added 10 unit tests of the auxiliary redim method. They are very simple and the core redim method is well tests already.

@philippjfr I think this PR is ready for review. I hope to get it merged ASAP as I have some nice improvements to DynamicMap usage that builds on top of this.

@jlstevens jlstevens added the ready label Apr 17, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 17, 2017

Looks good, I would have considered subclasses that override the __call__ but this is fine too. Happy to merge when tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Apr 17, 2017

I would have considered subclasses that override the __call__

Agreed. I would definitely split it up into subclasses if the current redim __call__ method gets any more complicated than it is now i.e if we find ourselves wanting new functionality or needing a new mode.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Apr 17, 2017

Okay sounds good, merging.

@philippjfr philippjfr merged commit f2bf8d6 into master Apr 17, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 78.934%
Details
s3-reference-data-cache Test data is cached.
Details

@jlstevens jlstevens referenced this pull request Apr 17, 2017

Merged

Improve DynamicMap usability and deprecate sampled mode #1305

6 of 6 tasks complete

@philippjfr philippjfr deleted the redim_improvements branch Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment