Skip to content
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

fix minorticks #4528

Merged
merged 11 commits into from
Nov 21, 2022
Merged

fix minorticks #4528

merged 11 commits into from
Nov 21, 2022

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Nov 19, 2022

Fix #4508.

minorticks is very confusing since the name suggests the number of ticks, not the number of intervals which is n° ticks + 1).
==> refused as of "breaking".

As @ryofurue mentioned, the current behavior is very much inconsistent.
==> apparently, being inconsistent is acceptable.

This PR fixes these problems:

using Plots

myplot(ys; kw...) =
  plot(ys; ylims=(-0.2,1.2), yticks = 0:0.2:1, kw...)

ys = rand(Float64, 10)
myplot(ys; minorgrid=true, yminorticks=:auto)    # default number of minor intervals (5) - ticks (4)
myplot(ys; minorgrid=true, yminorticks=true)     # default number of minor intervals (5) - ticks (4)
myplot(ys; minorgrid=true, yminorticks=:none)    # no minor ticks
myplot(ys; minorgrid=true, yminorticks=nothing)  # no minor ticks
myplot(ys; minorgrid=true, yminorticks=false)    # no minor ticks
myplot(ys; minorgrid=true, yminorticks=0)        # 0 intervals - 0 minor ticks (makes no sense though)
myplot(ys; minorgrid=true, yminorticks=1)        # 1 interval  - 0 minor ticks
myplot(ys; minorgrid=true, yminorticks=2)        # 2 intervals - 1 minor tick
myplot(ys; minorgrid=true, yminorticks=3)        # 3 intervals - 2 minor ticks
myplot(ys; minorgrid=true, yminorticks=4)        # 4 intervals - 3 minor ticks
...

myplot(ys; minorgrid=false, yminorticks=:auto)   # no minor ticks with default `:auto`
myplot(ys; minorgrid=false, [...])  # same as above

I've modified the lens example to take 1 minor tick (and hence regress on test examples), since minorticks wasn't tested before with an integer argument.

PyPlot has been adjusted since AutoMinorLocator uses the number of intervals.

@t-bltg t-bltg force-pushed the minorg branch 3 times, most recently from c3a80fa to 9d474a8 Compare November 19, 2022 10:29
@t-bltg t-bltg force-pushed the minorg branch 3 times, most recently from 8484d2b to 096bd97 Compare November 19, 2022 11:27
@t-bltg t-bltg added bug enhancement improving existing functionality labels Nov 19, 2022
@codecov
Copy link

codecov bot commented Nov 19, 2022

Codecov Report

Base: 91.05% // Head: 91.08% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (fcbd4f0) compared to base (b75deeb).
Patch coverage: 94.73% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4528      +/-   ##
==========================================
+ Coverage   91.05%   91.08%   +0.02%     
==========================================
  Files          40       40              
  Lines        7673     7683      +10     
==========================================
+ Hits         6987     6998      +11     
+ Misses        686      685       -1     
Impacted Files Coverage Δ
src/args.jl 84.70% <ø> (ø)
src/backends/pgfplotsx.jl 89.00% <ø> (+0.18%) ⬆️
src/examples.jl 97.26% <0.00%> (-1.36%) ⬇️
src/axes.jl 90.54% <100.00%> (+0.39%) ⬆️
src/types.jl 100.00% <100.00%> (ø)
src/utils.jl 93.47% <0.00%> (-0.04%) ⬇️
src/plotattr.jl 96.00% <0.00%> (+0.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@t-bltg
Copy link
Member Author

t-bltg commented Nov 20, 2022

@BeastyBlacksmith, ok ?

Copy link
Member

@BeastyBlacksmith BeastyBlacksmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I agree, but we can't change semantics of an attribute in a minor release

@t-bltg
Copy link
Member Author

t-bltg commented Nov 21, 2022

Here are my arguments for this to go in now as a bug fix (and not breaking):

  • the keyword minorticks is a currently a lie, and is in no way related to intervals as it claims;
  • it wasn't tested anywhere in our tests before this PR so technically it is not breaking anything (the lens ex was modified but on purpose for this PR);
  • the implementation was absolute non sense and totally inconsistent: I think the current is unambiguous (and I'd like someone to prove me wrong or give counter arguments - unless this happens, nothing should prevent this);
  • I consider the 2.0 argument as invalid: we are not following a schedule (timeline) here with deadlines or due dates. as such, this is equivalent to saying that this will never happen.

@t-bltg t-bltg changed the title modify minorticks to not be the number of minor intervals fix minorticks to not be the number of minor intervals Nov 21, 2022
@BeastyBlacksmith
Copy link
Member

  • the keyword minorticks is a currently a lie, and is in no way related intervals as it claims;

Your example in #4508 (comment) showed that it does what it says apart from edge cases which could very well be handled differently (which would indeed be a bugfix)

  • it wasn't tested anywhere in our tests before this PR so technically it is not breaking anything (the lens ex was modified but on purpose for this PR);

Thats not how SemVer works. That we don't have tests for something does not mean you can't break it. On the contrary, it increases the probability that you break something accidentally.
Its not about breaking tests, but about breaking someone elses code.

  • the implementation was absolute non sense and totally inconsistent: I think the current is unambiguous (and I'd like someone to prove me wrong or give counter arguments - unless this happens, nothing should prevent this);

That might be and bringing the implementation to match the documentation would be the way to go here.

  • I consider the 2.0 argument as invalid: we are not following a schedule (timeline) here with deadlines or due dates. as such, this is equivalent to saying that this will never happen.

That we don't have a timeline doesn't mean it won't ever happen. I'm open to talk about making a plan, but we also need to collect things that should go in a breaking release.

@t-bltg
Copy link
Member Author

t-bltg commented Nov 21, 2022

This makes absolutely no sense.
I understand the users being lost to these kind of arguments, and going elsewhere (I'm absolutely not thinking about Makie here).

I've reverted minorticks to be the number of intervals, even if giving minorticks = 0 makes absolutely no sense mathematically (there is always at least one interval between major ticks).

I've updated the julia code in #4528 (comment).
So I'm going to merge this pending CI since the description change of minorticks has been removed.

@t-bltg t-bltg changed the title fix minorticks to not be the number of minor intervals fix minorticks Nov 21, 2022
@t-bltg t-bltg removed the enhancement improving existing functionality label Nov 21, 2022
@t-bltg t-bltg merged commit 5fd4d60 into JuliaPlots:master Nov 21, 2022
@t-bltg t-bltg deleted the minorg branch November 21, 2022 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minorgrid=true interacts with yminorticks inconsistently
2 participants