-
Notifications
You must be signed in to change notification settings - Fork 32
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
Port to latest holoviews #688
base: master
Are you sure you want to change the base?
Conversation
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.
Those changes all look good to me, thanks! @jlstevens, can you also review and maybe think about the remaining issue?
The remaining issue is down to changes in the compositor in holoviews. For now, I would recommend deleting that line: the compositor is just a convenience and you can achieve the equivalent visualization manually. It is better to get the data out and visualize it the way you please than having to deal with an unnecessary error. Updating that line to work with the current state of the compositor should be possible but that would require some thought and possibly some extra work too. |
"other" argument. This change catches this case.
value_dimensions -> vdims Correct arguments passed to Histogram
Fixed integration of Tk with IPython >= 5.x Fixed matplotlib crash with Tk All unit tests pass if nosetests pointed to unit folder
I added some more changes that I made. These solve:
Unfortunately there are still two serious problems that i do not know if I will have time to address:
|
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.
Nearly all of those changes look good, thanks! But there are a few problematic ones...
matplotlib_imported=True | ||
except ImportError: | ||
matplotlib_imported=False | ||
|
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.
Ooh, seeing anyone change this code just raises the hairs on the back of my neck! There have been so, so many issues over the years from Matplotlib choosing the wrong backend, that I am very reluctant ever to change this code. Are you certain that when used in batch mode (no Tk), it still works?
# Change in holoviews | ||
if type(other) == str: | ||
return False | ||
raise e |
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.
Can you explain the motivation for this change? When will a Specification ever be compared to a string?
# Need to load TkAgg before any calls to pyplot in the imports | ||
import matplotlib | ||
matplotlib.use("TkAgg") | ||
|
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.
This doesn't seem correct; tests cannot assume that Tk is available in that run.
@@ -531,7 +531,7 @@ def quit_topographica(self,check=True,exit_status=0): | |||
except: | |||
pass | |||
|
|||
self.message("Quit selected%s" % ("; exiting" if self.exit_on_quit else "")) | |||
# self.message("Quit selected%s" % ("; exiting" if self.exit_on_quit else "")) # Removed because does not restore prompt |
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.
Not sure what you mean?
That sounds like a serious problem that should be reported in a separate issue with a reproducible test case (even if you have to say "run this code 100 times and it typically has floating point errors 5% of the time", or something like that).
I would guess that the plots in the GCAL tutorial have polar orientation map plots that depend on the Compositor that you have commented out. @jlstevens, what do you propose should happen to that type of plot? |
@fcr I'll probably have a better idea how to resolve this if you could post the output of |
@jlstevens.
|
BTW the problem does not rely on Compositor that was commented out. |
I didn't study the entire traceback, but it does mention Compositor, so I would guess this error is just to do with Compositor not being defined (as it's commented out). |
I made my previous comment after I restored the Compositor. Made no difference. Not surprizing since the Compositor is keyed to cog and this is a general problem. |
I think this is too trick to debug from the traceback alone so I'll have a go at reproducing the bug soon. I suspect it might be to do with how normalization information used to be stored but I might be wrong about that... |
These changes allows topographica to run with the head version of holoviews. These requires my push request for featuremapper.
The one thing that that does not run correctly is the composition:
CoG_spec = "Image.X CoG * Image.Y CoG * Image.BlueChannel" XYCoG = chain.instance(group='XYCoG', name='XYCoG', operations = [image_overlay.instance(spec=CoG_spec), factory.instance()]) Compositor.register(Compositor("Image.X CoG * Image.Y CoG", XYCoG, 'XYCoG', 'display'))
defined in topo.analysis. As is, it causes an error when trying to display an overlay of CoGs.
If this is deleted overlays appear without an error. Any attempt that I made to fix this avoided the error, but only displayed outlines of the images.