-
Notifications
You must be signed in to change notification settings - Fork 89
No Plots.jl #235
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
No Plots.jl #235
Conversation
|
Yeah this is a good idea, will make loading quite a bit faster for those who do not want to plot. I think there are a lot of plot functions in pidplots that are not yet converted to recipies that would also need conversion ControlSystems.jl/src/pid_design.jl Line 35 in 1e113ca
|
1 similar comment
Change plots->recipesbase Fix gangoffour margin line
drops the last usages of Plots.jl
|
I've decided to push this over the finish line # Before
julia> @time using ControlSystems
11.099141 seconds (29.88 M allocations: 1.968 GiB, 7.44% gc time, 29.25% compilation time)
# Removing Plots.jl
julia> @time using ControlSystems
7.169636 seconds (20.90 M allocations: 1.369 GiB, 5.69% gc time, 29.55% compilation time)a sizable improvement to the loading time. The following is a list of dependencies that goes away with the change :) |
Codecov Report
@@ Coverage Diff @@
## master #235 +/- ##
==========================================
+ Coverage 85.70% 86.81% +1.10%
==========================================
Files 31 31
Lines 3226 3239 +13
==========================================
+ Hits 2765 2812 +47
+ Misses 461 427 -34
Continue to review full report at Codecov.
|
Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se>
|
Something failed when generating plots. See the log at https://github.com/JuliaControl/ControlExamplePlots.jl/actions/runs/1727639720?check_suite_focus=true for more details. |
|
The plot bot fails with
which is expected, I've run and looked at the plots locally and they look as expected |
albheim
left a comment
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.
LGTM
|
Does it look reasonable even with MIMO systems? |
|
With #605 it looks like this |
|
Nice. I think it was actually vector of systems that was tricky to solve, but there are other ways to do that now i guess. |
|
The vector of systems is handled using the same mechanism as which is a quite okay solution |
|
Played around a little with plotting on this branch and found some odd behaviours. Though testing on last release they also appeared, so since they seem unrelated to the PR I will add them in separate issues instead of here. |
|
try the #605 branch, some updates there |

It seems that we have removed every dependency on Plots.jl with this fix. We Still have to update examples, manual and manual generation, but we should consider getting rid of Plots.jl completely like this.
The only downside is that the user has to write
using Plotsbefore using any of our plot methods.