Skip to content

dynamic mapping configuration for issue #28772 - #28776

Closed
crspencer11 wants to merge 2 commits into
matplotlib:mainfrom
crspencer11:chrispencer-fix-get-tick-params
Closed

crspencer11 wants to merge 2 commits into
matplotlib:mainfrom
crspencer11:chrispencer-fix-get-tick-params

Conversation

@crspencer11

@crspencer11 crspencer11 commented Sep 2, 2024 •

Copy link
Copy Markdown

PR summary

Closes #27416: static method axes.get_tick_params() never returned labelbottom or labeltop. Fixed by dynamically updating shared key-value pairs with axis specific ones.

PR checklist

@crspencer11
crspencer11 marked this pull request as ready for review September 2, 2024 18:36

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@oscargus

oscargus commented Sep 3, 2024

Copy link
Copy Markdown
Member

The CI complains as you have tabs and we use spaces, but that should be an easy fix. The code looks good otherwise.

If you can please add a test that checks that the outcome is at expected, it would be great! In that way we can avoid breaking it later...

@crspencer11

crspencer11 commented Sep 3, 2024 •

Copy link
Copy Markdown
Author

The CI complains as you have tabs and we use spaces, but that should be an easy fix. The code looks good otherwise.

If you can please add a test that checks that the outcome is at expected, it would be great! In that way we can avoid breaking it later...

Fixed the first issue as I forgot to install pre-commit checks. The testing, however, is throwing this error:

ImportError: cannot import name '_c_internal_utils' from 'matplotlib' from lib/matplotlib/init.py. I am perhaps missing a compiler installation?

Also, after recommitting, you can see many of the failed jobs errored on some sort of OS jobs. Is there some sort of container that I have to spin up?

Comment thread lib/matplotlib/axis.py Outdated
Comment on lines +1106 to +1115
'top': 'tick20n',
'bottom': 'tick10n',
'labelbottom': 'label10n',
'labeltop': 'label20n',
},
'y': {
'left': 'tick10n',
'right': 'tick20n',
'labelleft': 'label10n',
'labelright': 'label20n',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You have replaced all the "O"s here with zeros. I think that is why the tests are failing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I should probably have given more explanation here.

You can see the details of the failures by clicking the "details" link on the right of any of the failed jobs below. The actual failure line looks like this. The values in this dictionary ultimately get passed as keywords to the Tick constructor, but since you change the spelling (replacing the letter 'O' with a zero) they are not recognised keywords so end up in kwargs. Everything in kwargs gets shortened and passed to the Line2D constructor, where it is also not recognised, hence the error.

@lentil32

lentil32 commented Sep 8, 2024 •

Copy link
Copy Markdown

crspencer11#1
@crspencer11 please refer to this pr

@crspencer11

crspencer11 commented Sep 8, 2024 •

Copy link
Copy Markdown
Author

crspencer11#1

@crspencer11 please refer to this pr

not quite sure what this accomplishes? if it is to fix the linting issue with tabs over spaces I already recommitted that after installing pre-commits.

@lentil32

lentil32 commented Sep 9, 2024 •

Copy link
Copy Markdown

crspencer11#1

@crspencer11 please refer to this pr

not quite sure what this accomplishes? if it is to fix the linting issue with tabs over spaces I already recommitted that after installing pre-commits.

since you mistakenly modified variable names, that pr reverts it.

The names of variables are xxxOn, not xxxx0n (you changed alphabet 'O' to digit '0').

image

@rcomer

rcomer commented Sep 12, 2024

Copy link
Copy Markdown
Member

Note, I have updated the linked issue to an older one, as the recent one was flagged as a duplicate.

@crspencer11

Copy link
Copy Markdown
Author

crspencer11#1
@crspencer11 please refer to this pr

not quite sure what this accomplishes? if it is to fix the linting issue with tabs over spaces I already recommitted that after installing pre-commits.

since you mistakenly modified variable names, that pr reverts it.

The names of variables are xxxOn, not xxxx0n (you changed alphabet 'O' to digit '0').

image

I see

@crspencer11
crspencer11 force-pushed the chrispencer-fix-get-tick-params branch from 67d1da7 to 0d87a5e Compare September 17, 2024 18:21
@rcomer

rcomer commented Dec 7, 2024

Copy link
Copy Markdown
Member

Closing in favour of #29249.

@rcomer rcomer closed this Dec 7, 2024
@melissawm melissawm moved this from Waiting for author to Done in First Time Contributors Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

[Bug]: get_tick_params on xaxis shows wrong keywords

4 participants