Skip to content

Conversation

@t-bltg
Copy link
Member

@t-bltg t-bltg commented Jun 18, 2021

Superseding #3547, required patch: JuliaPlots/PlotUtils.jl#115.

Test cases:

using Plots; gr()

for (i, (start, stop))  enumerate([(-1, 2), (-6, 5)])
    x = y = Float64[1]
    for j  start:stop
      append!(x, 2 * 10.0^j:10.0^j:10.0^(j + 1) |> collect)
    end
    plot(
      x, y, axis=:log10, minorgrid=true, legend=:none,
      shape=:cross, ms=2, lw=.5, dpi=200
    )
    png("loglog-$i")
end

Output:
loglog-1
loglog-2

Fix #3318
Fix #696
Fix #458

@t-bltg
Copy link
Member Author

t-bltg commented Jun 25, 2021

@BeastyBlacksmith, pinging you on this one for review / merge: if we could move forward that'll be great ;)

@t-bltg t-bltg marked this pull request as draft July 4, 2021 19:05
@t-bltg t-bltg marked this pull request as ready for review July 4, 2021 19:27
@t-bltg
Copy link
Member Author

t-bltg commented Jul 4, 2021

Fixed the only minor issue with example 5 (ref5): 1 decade (minimal number of major ticks = 2) is enough for log scales.

Old

ref5

New

ref5

Note: the CI will be significative only once JuliaPlots/PlotUtils.jl#115 is merged.

t-bltg added a commit to t-bltg/PlotReferenceImages.jl that referenced this pull request Jul 4, 2021
@t-bltg t-bltg requested a review from daschw July 4, 2021 19:55
@t-bltg t-bltg requested a review from BeastyBlacksmith July 4, 2021 20:00
@t-bltg t-bltg added GR enhancement improving existing functionality labels Jul 5, 2021
@t-bltg t-bltg marked this pull request as draft July 5, 2021 13:18
@t-bltg t-bltg marked this pull request as ready for review July 5, 2021 16:41
@t-bltg t-bltg marked this pull request as draft July 5, 2021 16:44
@t-bltg t-bltg force-pushed the log_bis branch 2 times, most recently from 55f3c0d to fd590bb Compare July 5, 2021 20:43
@t-bltg t-bltg closed this Jul 5, 2021
@t-bltg t-bltg reopened this Jul 5, 2021
@t-bltg t-bltg closed this Jul 5, 2021
@t-bltg t-bltg reopened this Jul 5, 2021
@t-bltg t-bltg marked this pull request as ready for review July 5, 2021 21:56
@t-bltg
Copy link
Member Author

t-bltg commented Jul 5, 2021

Ok, so after merging JuliaPlots/PlotUtils.jl#115 and triggering a new release of PlotUtils, the only failing test is ref5 as expected.

@BeastyBlacksmith or @daschw if I could have your approval, we can merge JuliaPlots/PlotReferenceImages.jl#97 and this PR.

@BeastyBlacksmith
Copy link
Member

how did ref5 look like with minmal number of ticks = 4?

@t-bltg
Copy link
Member Author

t-bltg commented Jul 6, 2021

how did ref5 look like with minmal number of ticks = 4?

$ julia ref5.jl
┌ Warning: No strict ticks found
└ @ PlotUtils [...]/PlotUtils/n4I4Q/src/ticks.jl:295
┌ Warning: No strict ticks found
└ @ PlotUtils [...]/PlotUtils/n4I4Q/src/ticks.jl:295

ref5

@BeastyBlacksmith
Copy link
Member

I suppose this is because it tries to find integer exponents. Can we have some kind of look for integer exponents -> if not enough ticks present look for mulitples of 1/2 -> if not enough ticks found look for multiples of 1/4 etc. ?

@t-bltg
Copy link
Member Author

t-bltg commented Jul 6, 2021

I suppose this is because it tries to find integer exponents. Can we have some kind of look for integer exponents -> if not enough ticks present look for mulitples of 1/2 -> if not enough ticks found look for multiples of 1/4 etc. ?

What's wrong with requiring less major ticks ? It is justified by the choice of log scaling.

Non-integer powers do not bring information to the plot or the user: 10^-.5 ? you need a calculator, I am not in favor of it.

@BeastyBlacksmith
Copy link
Member

I think the plot looks a bit lost with only two ticks. But surely that is subjective, probably would look okay with minorticks. Overall I am okay with the changes

@t-bltg
Copy link
Member Author

t-bltg commented Jul 6, 2021

I think the plot looks a bit lost with only two ticks. But surely that is subjective, probably would look okay with minorticks. Overall I am okay with the changes

It is subjective yes, see also the examples in https://fr.mathworks.com/help/matlab/ref/loglog.html.

Don't count on minor-ticks there, we need at least 3 major ticks because of this

if length(ticks) > 2
.

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #3577 (9ba89ec) into master (7b0066c) will decrease coverage by 1.20%.
The diff coverage is 40.88%.

❗ Current head 9ba89ec differs from pull request most recent head 16e41f0. Consider uploading reports for the commit 16e41f0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3577      +/-   ##
==========================================
- Coverage   64.83%   63.63%   -1.21%     
==========================================
  Files          27       28       +1     
  Lines        6641     6855     +214     
==========================================
+ Hits         4306     4362      +56     
- Misses       2335     2493     +158     
Impacted Files Coverage Δ
src/backends.jl 61.06% <0.00%> (-2.25%) ⬇️
src/backends/gaston.jl 0.00% <0.00%> (ø)
src/utils.jl 52.80% <60.00%> (+0.22%) ⬆️
src/backends/gr.jl 89.12% <98.30%> (+1.10%) ⬆️
src/axes.jl 85.20% <100.00%> (+1.48%) ⬆️
src/backends/pgfplotsx.jl 60.94% <100.00%> (ø)
src/examples.jl 61.76% <100.00%> (+0.57%) ⬆️
src/init.jl 100.00% <100.00%> (ø)
src/layouts.jl 68.22% <100.00%> (+1.39%) ⬆️
src/pipeline.jl 89.63% <100.00%> (+0.57%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b0066c...16e41f0. Read the comment docs.

@t-bltg
Copy link
Member Author

t-bltg commented Jul 6, 2021

@BeastyBlacksmith

Ok this has be bothering me not to be able to have minorgrids if we have 2 major ticks, I dislike un-finished things.
I reworked the logic and here is ref5 when running tests:
ref5

Would that be ok ?

@t-bltg t-bltg force-pushed the log_bis branch 3 times, most recently from e0ad338 to ba17932 Compare July 6, 2021 17:11
t-bltg added a commit to t-bltg/PlotReferenceImages.jl that referenced this pull request Jul 6, 2021
@t-bltg t-bltg closed this Jul 6, 2021
@t-bltg t-bltg reopened this Jul 6, 2021
@t-bltg t-bltg closed this Jul 6, 2021
@t-bltg t-bltg reopened this Jul 6, 2021
@t-bltg t-bltg merged commit e994925 into JuliaPlots:master Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement improving existing functionality GR

Projects

None yet

2 participants