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

Feature/gonumplot #272

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Feature/gonumplot #272

wants to merge 4 commits into from

Conversation

JeroenMulkers
Copy link
Collaborator

This pull request changes how the plot in the web gui is created. Instead of calling gnuplot, the gonum/plot package will be used to create the plot.

The advantages of using gonum/plot over gnuplot are:

  • No need to spawn a subprocess
  • It removes the gnuplot dependency

The plot in the web gui is no longer created by a gnuplot subprocess.
The gonum/plot package is used to create the plot instead.
@godsic
Copy link
Contributor

godsic commented Oct 19, 2020

Looks neat, but I will give it a closer look.

@godsic
Copy link
Contributor

godsic commented Oct 22, 2020

@JeroenMulkers
I've noticed a few things that alarmed me a bit

  • gonum's plot pulls in many dependencies like golatex, gopdf, etc. My concern is that those might call some binaries that might not be available on the client side, in particular, on Windows machines. Could you please test on Windows and double check it works as expected?
  • I've noticed that for long running simulations I frequently get broken pipe errors if I open web interface on a remote machine. Maybe the figure update interval is a bit too short?
  • The default font is a bit too academic and contrasts with most browsers defaults. Gnuplot hardcoded Arial, but Sans-like fonts should also work.
  • I would prefer to have axes on all sides as it sometimes simplifies reading the figure data.

@kkingstoun
Copy link
Contributor

I can confirm that the gonum's plot works very well on Windows. I have also seen several times "broken pipe" errors.

@godsic
Copy link
Contributor

godsic commented Oct 22, 2020

@JeroenMulkers In addition, I would suggest to not add brackets to labels if units are empty.

@JeroenMulkers
Copy link
Collaborator Author

JeroenMulkers commented Oct 28, 2020

  1. The update interval of the plot can not be set explicitly. A possible solution is to cache to plot figure and give it a certain expiration date (e.g. 1s). This is what is done in commit #24c775e.

  2. I also changed the data format of the image from svg to png because the svg file can become quiet large (~Megabytes) when the amount of data increases, whereas a png file remains small (kilobytes).

These two changes drastically reduce the number of broken pipe errors. However, there are still disadvantages of using gonum/plot over gnuplot:

  • The line plots do not seem to be always correct if the plot is written to a png file. For this reason, only a scatter plot is shown
  • Lines are plotted correctly in the svg files, but these can become quiet big as mentioned and do not seem to support sans serif fonts.
  • As mentioned by @godsic , the gonum/plot library depends on other libraries which brings us closer to dependency hell
  • It is not possible to have axis on all sides

I will consider the attempt of changing the plot engine as a fun exercise, but for the above mentioned disadvantages I believe it is better to stick with gnuplot.

@godsic
Copy link
Contributor

godsic commented Nov 4, 2020

@JeroenMulkers I still think it would be a good idea to have go-native visualization, assuming the backend is properly maintained. gonum appears to be a good choice, but go-plotly might be a good alternative assuming we are using offline mode to avoid data breaches to 3rd parties.
In any case, @JeroenMulkers would you be so kind to split 24c775e into self-contained ones, so that, it would be easier to revive this feature?

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.

3 participants