Plotly: Enable specified contour values for ranges; warn otherwise #3757
Conversation
|
With this PR: x = y = -π:0.01:π
z = 1.01.*sin.(x).*cos.(y)'
levels = -1:0.5:1
plot(
[contour(x, y, z, levels=l, c=:rainbow)
for l in (length(levels), levels, collect(levels))]...,
grid=false
)
|
Codecov Report
@@ Coverage Diff @@
## master #3757 +/- ##
==========================================
+ Coverage 63.05% 63.14% +0.09%
==========================================
Files 28 28
Lines 7064 7076 +12
==========================================
+ Hits 4454 4468 +14
+ Misses 2610 2608 -2
Continue to review full report at Codecov.
|
The are no image comparison test for backends other than But you can try running and compare the examples against the using Plots; plotlyjs()
for i ∈ 1:length(Plots._examples)
i ∈ Plots._backend_skips[:plotlyjs] && continue
try
Plots.reset_defaults()
map(x->Base.eval(Main, x), Plots._examples[i].exprs)
png("plotly-$i")
catch err
@error "" bt=(err, stacktrace(catch_backtrace()))
end
end |
|
I think we got 3 different types of tests:
For the given case here I'd recommend to add some of the latter |
|
Thanks for the explanations, both.
Okay—I have done this and everything looks the same as in the docs. I guess this is expected because the only change is to the contour handling for Plotly, and the example does not specify any contour number or levels. The caveat to this is that examples 32, 47 and 50 produce empty files and the
I'm not totally sure how these tests work, but I guess I should add a file
I can't see any backend tests for Plotly at the moment. Should I add a |
|
In starting to write these tests, I realise I don't understand what the
What are the number of x-levels and y-levels for a contour plot? I don't understand what that could mean here. Trying the example above with Just asking since I will need to update this PR to reflect. |
Using git blame, traces appear in cf4fcf3, #99 and #101, although I did not find explicit reference to
My advice on this is to add a minimal |
If there is no processing of the argument on the plots-side than adding a test is not really needed, but wouldn't hurt either. Those tests really check if the
Thats correct and also fine to only test what is done in this PR. Hopefully it will grow over time. Thanks a lot for your effort! |
I think we can remove that if its not supported by any backend |
Plotly does not support arbitrary contour values, but can plot specific contours if they are an equally-spaced range. This commit implements the plotting of contours if the `levels` keyword argument is passed an `AbstractRange`, or if a set of arbitrary values are passed. In the latter case, however, since this is not supported by Plotly, a range based on the first and last values of the collection passed in is created and used to define the contours. A warning is then issued to the user. Otherwise, any other types are assumed to be number-like and adjusted as previously. Note that Plotly does not guarantee the exact number of contours will be used. This partly addresses JuliaPlots#3356. See plotly/plotly.js#4503 (Plotly issue tracking the ability to set arbitrary contours).
- Test that the `levels` keyword argument is correctly stored. - For the Plotly (and PlotlyJS) backend, test that `levels` is correctly converted to backend-specific settings.
anowacki
force-pushed
the
an/plotly-levels
branch
from
6705b7e
to
f1c0c7e
Compare
|
I have updated the PR with tests and corrected the logic in converting In looking over this I realised that the different backends do different things if the input to |
Thanks. Can you also update
I'm supporting unification of the backends on this and error out on unsupported (not Integer nor AVec). |
No problem—I'll update shortly.
Sounds like a a good, user-friendly solution to me and I'm happy to add that here or as a separate PR if that's more appropriate. Where should the check go? I haven't found any sort of central argument-checking place in the codebase so far, but likely I have missed it. Or perhaps it should go in RecipesPipeline? |
Thinking out loud, maybe in function check_supported_levels(series)
if !(typeof(series[:levels]) <: Union{Integer,Avec})
error("Unsupported message ...")
end
end, and modify the backends accordingly. |
I'd be happy to do that, but that suggestion does then still require quite a lot of code duplication in the backends—each one will separately need to call e.g. As a potential alternative, it seems like How about calling the function you posted @t-bltg (or equivalent) in |
Agreed, I haven't checked if all backends behave in the same way on keyword |
The `levels` keyword argument does not actually support
values of type `NTuple{2,Integer}` despite the existing documentation,
so remove the mention of this from `_arg_desc` and update the
description of what `levels` means.
Checking of the `levels` keyword argument will be moved out of individual backends' code, so we can assume that `levels` if present in `plotattributes` is either an `AbstractVector` or `Integer`.
This commit moves the check for the correctness of the `levels` keyword argument into `RecipesPipeline.preprocess_attributes!(::KW)` and calls the new function `check_contour_levels`, which in turn will throw an `ArgumentError` if the argument is not correct. To that end: - Remove the check for non-integer and non-vector values of `levels` from the PyPlot backend code, and - Add low-level tests for the new function and high-level tests for contour plots.
|
From a survey of the backends, only PyPlot previously checked what |
|
Note that I added a check that |
Safe assumption, we'll see if any issue pops up on this matter. |
This PR is meant to help address #3356. It only does partially.
Note that there are no tests yet, though things seem to be
working locally for me. Guidance on how to add tests would
be greatly appreciated.
Plotly does not support arbitrary contour values, but can plot
specific contours if they are an equally-spaced range. However,
the current behaviour is to simply error if a range or collection
of values are passed via the
levelskeyword, since the currentimplementation (correctly) adds two to whatever is passed in
This commit implements the plotting of contours if the
levelskeyword argument is passed an
AbstractRange, or if a setof arbitrary values are passed. In the latter case, however,
since this is not supported by Plotly, a range based on the
first and last values of the collection passed in is created
and used to define the contours. A warning is then issued to
the user.
Otherwise, any other types are assumed to be number-like and
adjusted as previously. Note that Plotly does not guarantee
the exact number of contours will be used.
See plotly/plotly.js#4503 (Plotly issue
tracking the ability to set arbitrary contours).
Possible things to look at are:
to hook to do this properly, since this is a partial limitation
of a backend, not a complete absence of a feature.
right thing to do, even with a warning, or we should instead
error when passed a tuple or vector. Or should we check that
the values therein are actually equally spaced, in which case
we could plot those exact contours?