Open arturgower opened 6 years ago
Have a look at the first function in plots.jl. plot(SimulationResult) works, but plot(SimulationResult, linetype=:contour) and plot(SimulationResult, linetype=:field) do not...
Plots is quite complicated because linetype can be define in several ways, in commit 7a66332 I use
plotattributes[:xlims]
to access xlims as defined in any other way. This may help to make the function more robust.
Another thing to think about is, should we allow surface type plots? These can accept vectors of x, y and z with no real structure and infer a triangular interpolation. Using something like this would mean we wouldn't have to assume that the user provided x
vector is structured nicely. It should also be possible to do this for a contour plot, but I can't seem to do it.
Here's a quick example to show surface plots with unstructured sample points:
rx = 2.*(rand(1000).-0.5)
ry = 2.*(rand(1000).-0.5)
surface(rx,ry,rx.^2.+ry.^2)
A simulation result can have any number of angular frequencies or positions. What if we pretty much ignore linetype and have plot(simresult, x_vec, ω)
where the user can put
plot(simresult, :, ω) # Plot all positions for one angular frequency, i.e. contour
plot(simresult, x, :) # Plot one position for all angular frequencies, i.e. line
We can then detect :
with something like (:) === Colon()
. User could also pass in vector of Floats, vector of SVectors or vector of indices. If both were vectors then I'm not sure what we would do.
PS: are ω and x the wrong way around everywhere? Does it make sense to have angular frequency or position first?
I think having a plot, like surface, that takes any x is awesome. But really it needs to be a 2D plot, for quality and to show particles/sources. Compare the below surface and contour:
Do you think that perhaps plot(sim, x, ω)
would be better for surface plot? That way the user can pass any x
and ω
(and not pass the indices, which is a bit awkward). I think that would be nicer.
To plot FrequecySimulationResult
and TimeSimulationResult
, I'm not sure. I'm thinking that plot(simresult, x_indices, ω_indices)
is a bit awkward. For a user that knows the x_indices
they want, we can provide a function like simres2 = deepcopy(simres; x_indices = ...)
that creates a copy with only those x_indices, then the user can just plot(simres2, ω)
, where plot
finds the closest simres.ω
to ω
and plots that. Also as an optional argument plot(simres, ω; x_indices = ..)
would be good too!
For now, how about we implement plot(simres, ω; seriestype=surface, x_indices = : )
and plot(simres, ω; seriestype=contour, x_indices = : )
. If the user provides the wrong format for seriestype=contour
we can throw an error.
Btw, was there a reason you specialised the plot functions to FrequecySimulationResult
? I've converted it back to SimulationResult
, which needed only a small tweak.
Here is my suggestion. I know you don't love keywords! But for me this the most useful and intuitive option:
plot(simres; kws..)
shows line plots, where kws can include x_indices =..
, x=Vector{AbstractVector}
and ω_indices = ...
.plot(simres, ω; kws..)
is either a surface (default) or contour plot, depending on kws. Advanced user can further restrict x_indices = ..
, but it would be much nicer to pass, say, bounds = Rectangle(...)
. Hard to imagine using x_indices = ..
. What do you think?
And sorry it currently does not work and I couldn't find why! I have to run off and enjoy Kyoto now.
Contour and surface are entirely a matter of taste, and we should really be able to do both whatever the user gives us. I just can't seem to get contour to accept unstructured data or, equally, extract the contour plot embedded in surface. I think without significant effort, for now we are limited by what plots can cope with. Also, surface looks odd from above, it's only really useful from an angle.
The reason I specialised the function to frequencies was because getfield
is brittle and unclear, also any keyword arguments or text called ω don't make sense in the context of time. src/plot/plot.jl:7 shows the kind of error which is very easy to make and hard to spot. It's true that there are probably ways around all of these problems. I tend to err on the side of defensive programming, but in this case I don't have strong feelings.
Absolutely, indices are not the most natural way of interacting for a user, I intended to implement a wrapper function much like you have. Although I intended to make more use of dispatch rather than keywords, but I quite like how you've done it. I'm not generally a big fan of keywords if the same thing can be achieved without them, but for high level stuff like plotting they can make a lot of sense.
Yes, just adding some wrappers for your index function would be nice. I just didn't think of it at the time!
I fully support writing more robust functions that don't use getfield
too.
The cause of the error was the weirdest thing:
@recipe function myplot(x::Vector{Int}; t::AbstractFloat = 2.)
(x,t*x)
end
but then
plot([1:2])
>ERROR: MethodError: Cannot `convert` an object of type Expr to an object of type Symbol
but if you don't type declare the keyword t
, such as the below, then there would be no error.
@recipe function myplot(x::Vector{Int}, y::Int; t = 2)
(x,t*x)
end
that is, plot([1,2],2)
works.
I think the following should work:
plot(SimulationResult)
should return several lines plots for each xplot(SimulationResult, linetype=:contour)
should attempt a contour plot. This should work for bothTimeSimulationResult
andFrequencySimulationResult
. Note the functionrun(Simulation,shape)
can return bothTimeSimulationResult
andFrequencySimulationResult
, which can then be plottedplot(FrequencySimulation, ω)
should generate a contour plot.There might be a dispatch issue for the first two, as I'm struggling to get plot recipes to do this...