search

mbaz / Gaston.jl

A julia front-end for gnuplot.
MIT License
148 stars 30 forks source link

Tag a release? #132

Closed PallHaraldsson closed 4 years ago

PallHaraldsson commented 4 years ago

I found the line to fix the timeout (to 6 worked for me) but can't change just current tagged release: https://github.com/mbaz/Gaston.jl/blob/c1416798f8aed1aa8f842f45220990537a6c43b3/src/Gaston.jl#L29

I see that I do not get a timeout on master, and that line has moved, but strangely didn't look sufficient. Anyway, just release? If too much backport that one change?

PallHaraldsson commented 4 years ago

Master is slightly slower though (and it's the using part, note same Julia 1.5) not something the optimization directive in code would fix (while it only helps for first plot, the only assumed speed problem):

$ time julia-1.5 -e 'using Gaston; plot(randn(10^6),randn(10^6))' -O0

real    0m1.321s
user    0m1.683s
sys 0m0.517s

$ time julia-1.5 -e 'using Gaston; plot(randn(10^6),randn(10^6))'

real    0m1.823s
user    0m2.172s
sys 0m0.529s
mbaz commented 4 years ago

Could you please give more details about the circumstances of the timeout?

mbaz commented 4 years ago

Also: on master, the timout values are now user-configurable, instead of fixed. That is why that line is no longer there. (But I just found a bug trying to set it -- I need to look into that).

mbaz commented 4 years ago

In my local master branch, I just updated the default timeout to 6. I've also fixed the bug in the set command, so users now can set their own timeouts properly.

PallHaraldsson commented 4 years ago

Could you please give more details about the circumstances of the timeout?

That's simple, it's just the plot:

plot(randn(10^6),randn(10^6))

as seen here: https://github.com/mbaz/Gaston.jl/pull/131#issuecomment-614651020

Yes, I was using julia 1.5 (and non-master Gaston.jl) but if I recall got the same on julia 1.4. Should be easy to confirm.

PallHaraldsson commented 4 years ago

so users now can set their own timeouts properly.

I'm not sure (most) users actually want to set timeouts, I'm not sure about the why needed (as not done in other plotting packages?). If the timeout is high enough then this doesn't matter.

The README seems outdated: "Fast: time to load package, plot, and save to pdf is around five seconds."

I would either tag master as is (I wouldn't know about all changes there), or only with a change to the tagged version with only the timeout relaxed (as that version is a bit faster), since your package seems helpful, and the timeout a distraction (when there's no real big problem).

I didn't make a PR for the README, maybe it's intentional, with plots taking very varying amount of time?

Anyway, I would also commit my change in the other PR on docs. I think that's at least an improvement. I'm not sure if something else can be done, in case gnuplot isn't installed, e.g. on Windows. Probably it could be downloaded, and provided as a JLL package on all platforms, but don't to that on my account...

mbaz commented 4 years ago

@PallHaraldsson I appreciate all your feedback and I'll merge your PR. I hope you don't feel ignored :)

At the moment I'm doing a large rewrite of Gaston's internals, and I want to get those done before I tag a new release. This new version has so many new user-facing features and internal code simplifications that I think it's worth to wait a few days more and do just one big release.

(Most the commits are still local to my computer -- I want any commits I upload to github to be good and not experimental)

Regarding timeouts: they should be invisible to most people, but sometimes, when running on a particularly slow computer or plotting huge amounts of data, a user can run into them. That's when I would point them to the ability to set custom timeouts. See for example https://github.com/mbaz/Gaston.jl/issues/118