-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Standardized constructor signature with dimensions as positional args #1946
Conversation
62b2ff3
to
cb67246
Compare
Are there backwards compatibility implications? Otherwise I support the idea. |
Not as far as I know but I have yet to go through all the errors. |
|
||
@property | ||
def deep_dimensions(self): return self.ddims | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we haven't used them in a long time so they should be ok to remove now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would think that, turns out Columnar_Data still had a reference:
3 error_hmap = hv.HoloMap({(i, j): hv.Image(j*np.random.randn(3, 3), bounds=extents)
4 for i, j in product(range(3), np.linspace(0, 1, 3))},
5 key_dimensions=['Observation', 'noise'])
Also addresses #1619 |
cb67246
to
17d8b6e
Compare
Okay, I've now gone through everything. There were two causes for errors:
I think better exception messages will be sufficient to address both cases. |
Sounds reasonable, but be sure to link to this PR in the changelog section on backwards compatibility. |
dcf8101
to
6b68d75
Compare
All sounds good to me! I definitely wouldn't want bounds to be passed by position to |
a5b0dfd
to
75a0bac
Compare
Tests now passing, I suppose I'll add some tests to ensure Elements follow this signature. |
75a0bac
to
b973187
Compare
Ready to merge once test pass. |
if edges is not None: | ||
self.warning("Histogram edges should be supplied as a tuple " | ||
"along with the values, passing the edges will " | ||
"be deprecated in holoviews 2.0.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something to mention in the next changelog/release notes. It will be good to get histogram working consistently with everything else.
Looks good to me. I made one comment about histogram but nothing that needs action. Merging. |
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. |
As suggested in #1938 this PR standardizes the constructors of Dimensioned objects such that
kdims
andvdims
are the second and third argument respectively for Elements andkdims
are the second argument for dimensioned containers (NdMapping, HoloMap, NdOverlay etc.). CurrentlyHistogram
andAnnotations
are the exceptions to this.