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

Remove strong dependency on matplotlib #872

Merged
merged 4 commits into from Mar 5, 2018
Merged

Conversation

@tammoippen
Copy link
Contributor

@tammoippen tammoippen commented Dec 13, 2017

PyNEST only uses pylab (matplotlib.pylab) in raster_plot and voltage_trace. In
tests, this matplotlib is mostly optional - except for test_sp/test_growth_curves.py.
This PR makes matplotlib completely optional for test.

Making matplotlib optional will greatly simplify maintaining the nest homebrew formula in homebrew-core, as homebrew-science will be deprecated.

@jakobj
Copy link
Contributor

@jakobj jakobj commented Dec 20, 2017

Nice! How about using the full try,except,else construct:

try:
    import pylab
except ImportError:
    pass
else:
    <plot stuff>

This isolated the exception a bit better ;)
Otherwise 👍 from me

@jakobj
jakobj approved these changes Dec 21, 2017
Copy link
Contributor

@jakobj jakobj left a comment

Beautiful :)

Copy link
Contributor

@heplesser heplesser left a comment

@tammoippen Thanks for this nice fix! But I am seriously wondering about why we have this plot in the test at all!? The code does, as far as I can see, nothing except check that plotting works. So I would remove it completely. I suppose the plot is here because we converted an example into a test at some point.

import pylab
from nest import raster_plot
except ImportError:
pass

This comment has been minimized.

@heplesser

heplesser Dec 21, 2017
Contributor

I would replace this with return and then have the else part as plain code.

This comment has been minimized.

@tammoippen

tammoippen Dec 22, 2017
Author Contributor

This would be even nicer.

Edit: On second thought: I am not so sure. This is exactly what the else-branch in a try-catch is supposed to do - perform the code that is only executable, when the try is successful. Putting a return there can easily be overlooked...

pylab.legend(('nest', sei.__class__.__name__))
pylab.savefig('sp' + sei.__class__.__name__ + '.png')
raster_plot.from_device(self.spike_detector)
pylab.savefig('sp_raster_plot.png')

This comment has been minimized.

@heplesser

heplesser Dec 21, 2017
Contributor

Why should we plot in a test at all?

This comment has been minimized.

@tammoippen

tammoippen Dec 22, 2017
Author Contributor

Look at this line. Apparently, the author of the test wanted to debug the tests...

This comment has been minimized.

@heplesser

heplesser Jan 5, 2018
Contributor

@tammoippen I suppose this is a matter of philosophy, but it makes some sense. To make the code more legible, I would suggest

  • to move the definition of the plot() method to right before
  • rename it _plot() to mark it as internal
  • add a docstring like "Can be called by tearDown() to cross-check tests..

This comment has been minimized.

@tammoippen

tammoippen Jan 15, 2018
Author Contributor

@heplesser

  • to move the definition of the plot() method to right before

Which version are you referring to? The complete _plot code in the try: block?

@tammoippen
Copy link
Contributor Author

@tammoippen tammoippen commented Feb 5, 2018

@heplesser Please can you clearify that for me:

@heplesser

   to move the definition of the plot() method to right before

Which version are you referring to? The complete _plot code in the try: block?

tammoippen added 3 commits Dec 12, 2017
PyNEST only uses pylab (matplotlib.pylab) in `raster_plot` and `voltage_trace`. In
tests, this matplotlib is mostly optional. This makes matplotlib completely optional
for test.
@tammoippen tammoippen force-pushed the tammoippen:rm-matplotlib branch from 6c81bc5 to d60e189 Feb 19, 2018
@tammoippen
Copy link
Contributor Author

@tammoippen tammoippen commented Feb 19, 2018

I applied the changed I suggest @heplesser asked me to do. This PR is hindering nest to be accepted in homebrew core since Dec 13, 2017. I get no replies since Jan 15. Can somebody please have a look?

Copy link
Contributor

@heplesser heplesser left a comment

@tammoippen We discussed this during our ongoing hackathon and concluded that test should not contain any plotting, and that "try import except pass" can be dangerous in tests. Could you explicitly remove all plotting-related code?

@tammoippen
Copy link
Contributor Author

@tammoippen tammoippen commented Mar 5, 2018

@heplesser Done as requested.

@heplesser heplesser merged commit 71f2f1f into nest:master Mar 5, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tammoippen
Copy link
Contributor Author

@tammoippen tammoippen commented Mar 5, 2018

Thanks! 🎉 🍰

@tammoippen tammoippen deleted the tammoippen:rm-matplotlib branch Mar 5, 2018
@tammoippen tammoippen mentioned this pull request Mar 6, 2018
4 of 4 tasks complete
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

4 participants
You can’t perform that action at this time.