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

Unified redim implementation with convenience methods #1302

Merged
merged 7 commits into from Apr 17, 2017
Merged

Conversation

@jlstevens
Copy link
Contributor

@jlstevens 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
Copy link
Contributor Author

@jlstevens 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.

@philippjfr
Copy link
Member

@philippjfr 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
Copy link
Contributor Author

@jlstevens 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
Copy link
Member

@philippjfr philippjfr commented Apr 17, 2017

Okay sounds good, merging.

@philippjfr philippjfr merged commit f2bf8d6 into master Apr 17, 2017
4 checks passed
@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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants