Rewrite Plot and axis-like types. #67
Conversation
This is a step towards fixing #44 (almost there!). 1. New user-facing plot/plot3 constructor is ```julia Plot([incrementa], [options], data, trailing...) ``` and similarly for `Plot3`. `incremental` defaults to `false`, as this seems to be the most common use case. Some validation is done on `data` (checking for type). This removes the `label` kwarg, and allows us to close #16. Examples now recommend an explicit `\addlegendentry`. Docstrings are added for everything, and examples are rewritten accordingly. 2. Axis-like code cleaned up a bit with macros. Variations on log axes added. Explicitly document that strings are emitted as is. 3. GroupPlot rewritten, allow multiple plots and empty \nextgroupplot, two examples added, one from the manual and one for multiple plots. 4. Replaced random examples with deterministic ones, perhaps stick to this and close #63? Minor indentation issues fixed. 5. Minor fixes for testing framework.
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
==========================================
+ Coverage 77.75% 81.28% +3.53%
==========================================
Files 9 9
Lines 472 481 +9
==========================================
+ Hits 367 391 +24
+ Misses 105 90 -15
Continue to review full report at Codecov.
|
src/axiselements.jl
Outdated
|
|
||
| Corresponds to the `\\addplot[3][+]` family of `pgfplot` commands. | ||
|
|
||
| The 1-4+ argument outer constructors `Plot` and `Plot3` should be used in user |
There was a problem hiding this comment.
I don't really understand this sentence.
src/axiselements.jl
Outdated
| Base.append!(plot::Plot, element) = (append!(plot.trailing, element); plot) | ||
|
|
||
| """ | ||
| Plot([incremental::Bool], [options::Options], data, trailing...) |
There was a problem hiding this comment.
The reason for not having incremental as a keyword is that we want it early, together with the options? Makes some sense but Plot(true, ...) will look kinda nondescriptive (and ugly), what is true there?
Perhaps we should just have a convenience constructor for this instead, in the same way we have for Plot3? Ploti, PlotInc or something?
There was a problem hiding this comment.
I'm also a little bit sad to see the keyword argument to give a legend to a plot go away. This is just such a common operation to do and having to do raw"\addlegendentry... doesn't feel nice. Would deprecating the label keyword into a legend or something be that bad?
There was a problem hiding this comment.
I agree, the boolean is kind of awkward, and I think that Plot[3]Inc is the best way to go forward, I will do that.
Regarding \addlegendentry: I realized that it is more complex than I thought, it can even take options etc, so adding a form for that, too.
I am against the keyword in Plot & friends, as that would break the very clear mapping between this package and pgfplots constructs.
There was a problem hiding this comment.
I am against the keyword in Plot & friends, as that would break the very clear mapping between this package and pgfplots constructs.
Ok, I can respect that.
| function (T::Type{<:AxisLike})(args::Vararg{PGFOption}) | ||
| T([], args...) | ||
| end | ||
| Return the corresponding LaTeX environment name. |
There was a problem hiding this comment.
Does this really need to be mentioned in the docs? It feels like this is an internal thing.
There was a problem hiding this comment.
I think this is a difference in our coding style; I like to document internal functions too, and indicate what belongs to the API with export and documentation. I can make it a comment if you prefer.
There was a problem hiding this comment.
Nah, just leave it like this then.
src/axiselements.jl
Outdated
|
|
||
| Same as [`Plot(::Bool, ::Options, ::PlotData, ...)`](@ref), but for 3D plots. | ||
| """ | ||
| Plot3(incremental::Bool, options::Options, data::PlotData, trailing...) = |
There was a problem hiding this comment.
Isn't
Plot3(args...; kwargs...) = Plot(true, args...; kwargs...)
enough to cover everything here?
src/axiselements.jl
Outdated
| const INCREMENTAL = false | ||
|
|
||
| "Types accepted by `Plot` for the field `data`." | ||
| const PlotData = Union{Coordinates, Table, TableFile, Expression, Graphics} |
There was a problem hiding this comment.
Is there ever a case where you would want to add a String to a Plot?
There was a problem hiding this comment.
Not that I know of. Expression pretty much covers those.
The `incremental` flag was removed from `Plot*` constructors in the API, replaced by `PlotInc` and `Plot3Inc`. A LegendEntry type was added. Minor fixes: - remove the INCREMENTAL constant, as it is now unnecessary - clarify docstrings - disabled docstring checks, as they lead to infinite loops for some reason
|
@KristofferC: I think I fixed everything, OK if I merge? |
|
Alright, we'll have to decide later wether to worry about deprecating old functionality. Running through all the examples in the PGFPlotsXExamples.jl and see what has broken could be a way to do that. |
Introduced a mixture of incremental and non-incremental forms, to demo both.
This is a step towards fixing #44 (almost there!).
and similarly for
Plot3.incrementaldefaults tofalse, as this seems to be the most common use case.Some validation is done on
data(checking for type).This removes the
labelkwarg, and allows us to close #16. Examples now recommend an explicit\addlegendentry.Docstrings are added for everything, and examples are rewritten accordingly.
Axis-like code cleaned up a bit with macros. Variations on log axes added. Explicitly document that strings are emitted as is.
GroupPlot rewritten, allow multiple plots and empty \nextgroupplot, two examples added, one from the manual and one for multiple plots.
Replaced random examples with deterministic ones, perhaps stick to this and close Use a seed for figures using random data. #63? Minor indentation issues fixed.
Minor fixes for testing framework.