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

add OS X matplotlib backend #115

Closed
wants to merge 4 commits into from
Closed

add OS X matplotlib backend #115

wants to merge 4 commits into from

Conversation

ronnnwu
Copy link

@ronnnwu ronnnwu commented Apr 7, 2017

From issue 114, I added MacOSX matplotlib backend, which allows interactive graphs to be displayed on Mac and it works for both the old matplotlib v1.4.3 and new v2.0.0.

@kengz
Copy link
Owner

kengz commented Apr 7, 2017

Hey thanks a lot for this.
We'll need to test this also on a full-experiment run using grunt to make sure it works there too.

If you have the full lab set up already and like check this, could you (the steps are described in Usage:

  • add a "dqn" to the config/default.json
  • run grunt

@ronnnwu
Copy link
Author

ronnnwu commented Apr 7, 2017

Thanks Mr. Keng!
On my mac I tried grunt for both matplotlib v1.4.3 and v2.0.0. By the way, there isn't a "default.json" file. I used the "config/example-default.json" file. It ran without error.
terminal.log.zip

The fix did not pass "codacy/pr" check. Is it because it doesn't like inline if/elif/else statement?

@kengz
Copy link
Owner

kengz commented Apr 8, 2017

the default.json and production.json normally gets created if the setup script is ran. But I'll include a step in the next installation guide so users would create them normally. Good catch!

Codacy is there to keep the codebase in check - to make code maintainable.
It's just complaining about a trailing whitespace at the end of the line; remove that extra space and it'll be fine

@kengz
Copy link
Owner

kengz commented Apr 8, 2017

Just tested. macosx backend blew up when ran with parallelism (full experiment).
If you restore the backend and try matplotlib version 1.4.3 as mentioned from #114 see if that works for you? If so that'd be a stabler fix, and I'll use it in the upcoming version lock

@ronnnwu
Copy link
Author

ronnnwu commented Apr 8, 2017

With v1.4.3 agg runs without error but interactive plots are not displayed. macosx works with v2.0.0 and showing interactive plots. But because of that, it fails in parallel grunt -prod. So I added a condition if sys_vars['RENDER'] not to show interactive plots. Now grunt -prod, grunt -best work fine. Ciao

@ronnnwu
Copy link
Author

ronnnwu commented Apr 8, 2017

Just find out grunt -best has a problem. The plot window (a super strong thread) cannot be closed normally. However, python main.py -t 5 -e dqn works just fine. Maybe grunt elevates matplotlib backend macosx to a higher priority that is not controlled by the grunt terminal.

@kengz
Copy link
Owner

kengz commented Apr 8, 2017

hmm, on quitting grunt it should quit the python subprocesses it spawns, and that should include matplotlib in py.

The more elegant solution is to use agg entirely with the right version of matplotlib. Since when running parallel plots we seldom wanna get blasted with tens of plots; when we wanna look we just open the individual plots in preview/sublime text and it gets updated.

We actually spent 1 weekend fixing all sorts of nasty backend bugs from the macosx backend and occasionally it blew up with a segfault. so, let's stick with the original backend as is, and I'll add the reasoning into the doc later as well. Thanks again for digging :)

@ronnnwu
Copy link
Author

ronnnwu commented Apr 9, 2017

Cool! For those who insist to use matplotlib v2.0.0, just comment out line 111 self.plt.ion() # for live plot, then agg works just fine. The effect is the same as using v1.4.3, hence no live plots.

@kengz kengz mentioned this pull request Apr 13, 2017
@kengz
Copy link
Owner

kengz commented Apr 15, 2017

Addressed in release v1.0.3

@kengz kengz closed this Apr 15, 2017
@lgraesser lgraesser mentioned this pull request Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants