-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Enable Dimension to accept a dict spec #5333
Conversation
Sorry for not getting back to you sooner! I've approved your workflow so we can run the tests. As for the proposal itself, I think it does make sense and that what you've written is accurate. However, the At any rate, I do think this is an important suggestion to consider. As HoloViews 1.15.0 is in a feature freeze (currently an rc), I will add it to the 1.15.1 milestone so we can think about including it in that release. |
Might an unrelated issue be interfering with the run of the CI tests? The failed tests appear unrelated to the changes in this pull request. The following 12 tests failed on all Python versions and all operating systems:
The following 5 tests also failed on Python 3.8 and 3.9 on Ubuntu:
|
I may have figured it out: #5344 updated the tests for datashader 0.14.1. Shall I rebase to incorporate that PR? |
That sounds very plausible and yes, please rebase. Given we just had a release, it would be good to catch up and we know all the tests have been passing on mater. |
Also test constructing MultidimensionalMapping with kdim given as dict.
Enables specifying Dimension with dict containing name and label.
Also, use name as default label.
Let the param system handle (1) a non-string name or label within a tuple and (2) a non-hashable label, which previously raised "TypeError: unhashable type" at the membership check.
Two helper functions no longer need to raise such an exception.
a849a7c
to
fcb3180
Compare
Not sure why the tests didn't re-run on this one. |
Right, I can't see the test status... |
I also see no tests, but let me know if anything is needed from me. |
Sorry for leaving this for so long @stanwest. If you could try pushing an empty commit or something to try to trigger the test suite that would be helpful. |
Codecov Report
@@ Coverage Diff @@
## master #5333 +/- ##
==========================================
+ Coverage 88.07% 88.12% +0.05%
==========================================
Files 301 301
Lines 61957 62108 +151
==========================================
+ Hits 54567 54735 +168
+ Misses 7390 7373 -17
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Looks good to me and tests seem to be passing but I'll have @jlstevens have the final say. |
Given that the imminent release is a minor version bump and should only be bug fixes, I propose that we merge this PR immediately afterwards. Overall it looks good and I look forward to trying it out on master! |
Thanks! |
Given that this PR was approved and HoloViews 1.15.1 was released, I'm going to merge it. |
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. |
This PR closes #5332 and is closely related to issue #2315 and PR #2332. Its main changes are as follows:
Dimension
with adict
.spec
is adict
that lacks a "name" key, raiseValueError
.spec
has an invalid type, raiseValueError
. (AlthoughTypeError
would be customary,ValueError
is backward-compatible with the behavior of helper functions aroundDimension
.)spec
is astr
.One could further refine this code as follows:
Dimension.presets
mapping (of PR Initial prototype for Dimension preset system #644) should remain; it appears unused internally (including by tests) and undocumented.collections.abc.Mapping
instead ofdict
, but other related code in Holoviews usesdict
.Dimension
class,dimension_name
function, orprocess_dimensions
function with an invalid type, raiseTypeError
instead of the currentValueError
.This change might make it easier for hvplot to accept
kdims
specified asdict
s instead ofDimension
s.