-
Notifications
You must be signed in to change notification settings - Fork 13
Adds basic plots #279
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
base: master
Are you sure you want to change the base?
Adds basic plots #279
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Overall very nice plots!
I do have some things I'd prefer differently though:
- by default I think the colour scheme should be the same as when plotting regularly, i.e. cycle through the default colours (blue, orange,...) with each extra plot added to a single figure. This way the colours are consistent with any other stuff the user might plot.
- I don't really like the grey shaded area in all plots. For a PDF for example just the line should be sufficient. Maybe for the pbox and interval it makes sense though.
- Instead of the
xlabelI think the variable name should actually go in the plot label. Currently if I plot two pdfs in one figure the second one will override the name of the first. Without passing colours, they will also be the same colour.
Please also add a few plots in suitable areas of the documentation. For example where the inputs are introduced.
|
|
||
| include("plotting/plot_recipes.jl") | ||
|
|
||
| include("util/fourier-transform.jl") |
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.
Why did you move these util includes down here? It's probably not to important, but I think they can stay at the top.
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.
You defined a lo(X::Real) and hi(X::Real) function but I don't think you use it anywhere.
Do you really need a function for the .lb and .ub of the Interval? To get the bounds of a Vector{Interval} you could instead use getproperty.(X, :lb) and getproperty.(X, :ub).
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.
Thanks for the review 👍 I think you're right
On the last one, I do find it convenient to have hi and lo functions for intervals and reals. It lets you ask for bounds on random sample of inputs, without knowing which elements are interval or real.
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.
In that case, keep them 👍
This PR adds the ability plot the following using plot recipes:
With plot recipes, various properties of the plots should be editable by the user as they would any standard plot with Plots.jl (i.e.
color=:red, will make the plot red)Eg, the following DEMO script (in
demo/plotting):