-
-
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
Fix shape and length reporting of gridded interfaces for Python 3.6 on Win 64 #2903
Conversation
holoviews/core/data/grid.py
Outdated
@@ -180,7 +180,7 @@ def shape(cls, dataset, gridded=False): | |||
if gridded: | |||
return shape | |||
else: | |||
return (np.product(shape), len(dataset.dimensions())) | |||
return (np.product(shape,dtype=np.intp), len(dataset.dimensions())) |
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.
Prefer to leave a space between arguments. Same applies below.
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.
Sorry, will fix those.
hv has flake8 whitespace checking turned off on travis. I also couldn't see any instructions for contributors about whitespace (whether it's important, and if so, what the rules are). And when I skimmed the hv code, I saw inconsistent whitespace in general, including spacing between arguments. So I think that makes it a bit tricky for contributors to get whitespace right, unless I've failed to see something?
If you can point me to the rules hv's using, I'll make a PR in docs and/or testing, plus clean up the existing whitespace.
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.
We don't have consistent rules (although I think we really should), but that's a whole argument to have. That said I'm very strongly in favor of separating arguments with whitespace and would be in favor of enabling this particular rule as a flake8 check immediately.
Looks good to me apart from a minor code formatting issue. |
Are the test failures my fault? I'll investigate if so (I couldn't run the test suite locally on the machine I was using for this PR). |
The errors are pretty bizarre, it looks like the Dimension |
Probably not, as I did most of this last week. I'll investigate! Also so far I only ran my own tests on these changes, not the existing HV tests (and I only ran on windows). |
Don't think anything would have changed since last week. Really not sure what could have changed though. |
Seeing |
I had a horrible thought on my way home...the errors somehow look like they'd be something more to do with recent param changes (rather than anything on this PR). However, I've just checked, and those changes to param were released before the end of June. There must have been successful tests of hv since then, right...? |
Hmm, my understanding had always been that holoviews is using param master on travis, but it's not. holoviews on travis gets its dependencies from either conda defaults, conda conda-forge, conda bokeh, or easy_install (via |
I've just pushed a commit to test that theory (I'm away for a while - will come back later). |
That's pretty worrying, any idea what might have changed in 1.7? |
I was imagining something to do with the namespace stuff. I should be able
to look later (I'm away now).
…On Tue, 31 Jul 2018, 18:20 Philipp Rudiger, ***@***.***> wrote:
param's coming from conda defaults. And looks like param 1.7 made it to
defaults 7 days ago. There hasn't been a successful travis build of
holoviews since then, as far as I can tell.
That's pretty worrying, any idea what might have changed in 1.7?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2903 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAHiUJt_VebQ2SmQ5fX4dfXT_-BTsW7ks5uMJHLgaJpZM4VoAK2>
.
|
That is worrying indeed. That said, wouldn't param 1.7 have been pulled in for the scipy pyviz tutorial? It seems to have worked fine then... |
Preliminary thoughts... The problem appears to be with the unpickled reference data, i.e. with backwards compatibility of pickles, rather than a problem with "current" hv or param. On unpickling, the recently added param namespace object is not being set up correctly from the state in the pickle (of the old param). Specifically, the param object's To test that idea out, I thought it'd be a one line fix in Parameterized.setstate to set self on the param object, but one or more hv things (e.g. LabelledData) appear to have their own independent state handling for pickle, and don't use Parameterized's, so I need to discuss if that's deliberate or an oversight. (Which I can do once I verify this really is the problem by creating a test case.) I also need to see what param's current policy is regarding backwards compatibility of pickles (and consider if that's how it should be). I used to make pickle loading backwards compatible with old pickles when param was part of topographica, but not sure if I ever moved any of that stuff to param itself, or what's happened since then... |
About the tests failing: moved to #2914 |
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. |
Part of #2895 (this PR just covers grid-based interfaces).
I used the below test files to check these changes. After switching to pyctdev and getting windows tests, these tests could be included in hv's test suite (but presumably re-written into class-based unit tests, which would reduce some boilerplate overlap). (Note for the future: the tests are intentionally set up to work on a machine with less than 4GB ram, for travis/appveyor.)
test_grid.py:
test_image.py:
test_iris.py:
test_xarray.py: