Bring minor and major ticks/tickabels to parity#74
Open
lucasb-eyer wants to merge 3 commits intompld3:masterfrom
Open
Bring minor and major ticks/tickabels to parity#74lucasb-eyer wants to merge 3 commits intompld3:masterfrom
lucasb-eyer wants to merge 3 commits intompld3:masterfrom
Conversation
Member
|
Looks like there's a conflict here, will leave it for you to fix before I review. Also, could you please rename Thank you! |
This will allow to render both major and minor gridlines in mpld3, which fixes the following issue: mpld3/mpld3#527 As per usual, disclaimer that I co-developed this with gpt-5.1-codex, having it figure out the issues and give implementation recommendations, with me testing, verifying, and tidying up the code. Here's what it has to say, especially wrt the change in API call: - include minor tick values/length and minor grid style in axis props so minor ticks/grids render in mpld3 - read grid color/linewidth/linestyle from tick kwargs (and rcParams fallback) instead of inspecting gridlines[0], avoiding the get_gridlines(which=…) API that isn’t available on matplotlib 3.10” Rationale for the kw/rc approach: `Axis.get_gridlines()` doesn’t accept `which` on matplotlib 3.10, so probing `gridlines[0]` for minor/major fails. Pulling style from the tick keyword dict (which matplotlib populates with `grid_*` fields when you call `ax.grid(...)`) plus `rcParams` defaults gives the same style without needing `get_gridlines(which=…)`, keeping compatibility and matching user-set grid styles. (I verified, indeed get_gridlines does not allow specifying which ones - seems like an omission in matplotlib API to me)
This will allow to render both major and minor gridlines in mpld3, which fixes the following issue: mpld3/mpld3#527 As per usual, disclaimer that I co-developed this with gpt-5.1-codex, having it figure out the issues and give implementation recommendations, with me testing, verifying, and tidying up the code. Here's what it has to say, especially wrt the change in API call: - include minor tick values/length and minor grid style in axis props so minor ticks/grids render in mpld3 - read grid color/linewidth/linestyle from tick kwargs (and rcParams fallback) instead of inspecting gridlines[0], avoiding the get_gridlines(which=…) API that isn’t available on matplotlib 3.10” Rationale for the kw/rc approach: `Axis.get_gridlines()` doesn’t accept `which` on matplotlib 3.10, so probing `gridlines[0]` for minor/major fails. Pulling style from the tick keyword dict (which matplotlib populates with `grid_*` fields when you call `ax.grid(...)`) plus `rcParams` defaults gives the same style without needing `get_gridlines(which=…)`, keeping compatibility and matching user-set grid styles. (I verified, indeed get_gridlines does not allow specifying which ones - seems like an omission in matplotlib API to me)
A few things were missing: - minor ticklabels altogether - major tick length setting - default tick length was arbitrary - make it same as matplotlib. This change brings both to parity, and makes sure they remain by using the same code for handling of most tick-related things, whether major or minor (except tickNr which only makes sense for major). After this, ticks and ticklabels have parity with standard matplotlib, I tested multiple scenarios (automatic vs manual setting and labeling, linear vs log axis, and similar) and compared against stock matplotlib. As usual, disclaiming I co-developed this with gpt-5.1-codex. We went through a lot of iterations here, both machine and me coding to get to this solution. Here's what it has to say: > Export and render minor ticks/grids with the same formatter/length handling as majors, including custom formatter overrides > Consolidate tick value/formatter application via a shared helper for both major and minor axes > Apply Matplotlib tick lengths to both major and minor ticks; keep minor labels hidden when only positions are set by default > Add tests for minor grid/tick export, minor label defaults, and major tick length”
lucasb-eyer
force-pushed
the
lb-ticklabels
branch
from
7403165 to
9f77a29
Compare
Contributor
Author
|
Fixed the conflict, it was code that needed manual reconciling with changes we merged in #69, but all makes sense. (Also had codex double-check my reconciliation.) Did the rename to avoid Now there is still the commit "Also export minor grid." twice here - it's coming from #73 and will disappear once I rebase after you merge #73. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Only the newest commit counts, the other two are from earlier PRs and will disappear on their merge and rebasing.
For some odd reason, I lost the whole nice commit message I wrote there... so I'll copy over the message from the corresponding mpld3 commit/PR, which explains it. This is the export part of it.
A few things were missing:
This change brings both to parity, and makes sure they remain by using
the same code for handling of most tick-related things, whether major or
minor (except tickNr which only makes sense for major).
After this, ticks and ticklabels have parity with standard matplotlib, I
tested multiple scenarios (automatic vs manual setting and labeling,
linear vs log axis, and similar) and compared against stock matplotlib.
As usual, disclaiming I co-developed this with gpt-5.1-codex. We went
through a lot of iterations here, both machine and me coding to get to this solution.
Here's what it has to say: