| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I believe that this can be one way to go, subject to the caveats in #23603 (comment) I think that there should be a copy of the dictionary returned and that there should be a comment that only those parameters deviating from the default (and/or rcParams?) are returned. (There should probably also be a version in mplot3d for Axes including a zaxis, but that can be extended in a different PR later.) |
Sorry, something went wrong.
|
Based on #23633 one may also opt for skipping the Axes method and only keep the Axis method. This would avoid any mplot3d issues and keep the API a bit cleaner. (But there is also the discoverability drawback of the Axis API.) |
Sorry, something went wrong.
|
@stefmolin sorry for letting this slip. I'm tempted to only add Axis.get_tick_params() for now. As opposed to the setter, which can set both Axis, and thus can save code, the getter has to resolve to exactly one axis and thus Axes.get_tick_params() is just a 1:1 rewriting of the axis access. We can always add the Axes method later if we consider it reasonable, but adding it now and taking away later is problematic. So, let's be defensive to start with. We should cross-ref in the docs of Axes.tick_params() and Axis.tick_params() which should reduce the discoverability issue a bit. A very central part of this, is communicating the meaning of these parameters: They are the default values, i.e. if new ticks are created (e.g. by panning or zooming) these values are used for the new ticks. This is in contrast with the actual tick values (the properties of the current tick objects), which can in principle be overwritten by the user. |
Sorry, something went wrong.
Works for me; I removed the extra method.
I updated the docstrings for this – let me know what you think.
Not sure what you mean by this. Are you referring to updating a docstring or code? |
Sorry, something went wrong.
The larger point is that we are not clear in distinguishing between the concept of a Axis-wide default tick style (stored in Axis._major/minor_tick_kw) and the styles of the actual ticks (stored in the individual tick instances). While it's debatable whether such a distinction is desirable, this is how our current implementation works and it shines through in parts of the API, which sometimes causes confusion to the users. I'm not sure whether documentation or code is the the right way forward. With documentation, we can explain everything in detail. OTOH we could also try to move the user away from interacting with or thinking about single ticks (#23372), which might make a detailed explanation of the topic unnecessary. This would be part code part documentation. On a side note, the latter would also be in line with ideas for internal refactoring that should increase performance (#5665 (comment)). |
Sorry, something went wrong.
|
One additional complication I just noted is that the internal names in _major/minor_tick_kw do not match the tick_params() keywords: matplotlib/lib/matplotlib/axis.py Line 948 in 3b79f63 We have to either translate back for get_tick_params() or clearly document that you get something else 😦 I'm sorry this is a bit of a mess. |
Sorry, something went wrong.
@timhoffm – Is there a preference? I can see these being confusing if you get them back: matplotlib/lib/matplotlib/axis.py Lines 1009 to 1016 in 3b79f63 The rest are probably fine. What do you think? |
Sorry, something went wrong.
|
Uh, this is a difficult decision. I'm tempted to translate back. I think the docstring https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.tick_params.html#matplotlib.axes.Axes.tick_params is the only user-visible place where we document what can go in (we currently accept more, but that somewhat coincidence). In that sense, get_tick_params should return the names as given there. This back-and forth converting is a bit annoying internally, but I think it is the most logical and consistent interface we can offer here. |
Sorry, something went wrong.
|
I added a reverse option to the _translate_tick_params() method and updated it to no longer alter the input dictionary. I'm using this in the forward and reverse directions in the test, so we can make sure both directions work going forward. |
Sorry, something went wrong.
|
@timhoffm - Any ideas what is going on for the docs per this comment above? Some additional questions on documentation for this PR:
|
Sorry, something went wrong.
Please add an entry to doc/api/axis_api.rst
You can try and add an Example section to the docstring. I would not make an addition to the Example Gallery. I hope this is correct, because it'S only from memory - please ignore if I'm wrong: It may be a bit confusing because AFAIR there are a few entries in _major_tick_kw by default. But we're stating that this only contains deviations from defaults (having "deviations from defaults" by default is a bit awkward). OTOH it may make sense to spell this out.
Yes, please.
No API change note, because our public API has not changed. |
Sorry, something went wrong.
|
Just close the PR. Open PRs anticipate a follow up action on the PR, even if that action is only "decide what to do". |
Sorry, something went wrong.
@timhoffm What do you mean by this? Was this meant for another PR? |
Sorry, something went wrong.
Sorry, something went wrong.
|
Yes, sorry! 🐑 |
Sorry, something went wrong.
Yeah, there are a few entries in there by default. I agree the phrasing is a little confusing. I modified the docstrings to indicate that we are returning the appearance of any new elements that will be added without calling out defaults – hopefully, that makes it easier to understand. I also added the example to the docstring and what's new entry. |
Sorry, something went wrong.
There was a problem hiding this comment.
Just a few documentation suggestions. I didn't read the history, but the docs refer to a mysterious "default style" which to me are the rcParam values, not the values currently on the axis. I think you are trying to differentiate between manual ticks and those styles automatically, but I think the note gets that across adequately. People don't often much with individual ticks, so I'd argue not to muddy the waters of the main docstrings with a rare case.
Sorry, something went wrong.
Co-authored-by: Jody Klymak <jklymak@gmail.com>
|
Should I also add the .. versionadded:: directive per the latest version of the PR template? Would this be for 3.7? |
Sorry, something went wrong.
|
@jklymak - I added the .. versionadded:: directive and tweaked the docstrings and example per your comments. |
Sorry, something went wrong.
| @@ -988,19 +1051,27 @@ def _translate_tick_params(kw): | |||
| 'labelright': 'label2On', | |||
| 'labeltop': 'label2On', | |||
There was a problem hiding this comment.
labelleft & labelbottom have the same axis key, the translation will therefore always return labelleft and labelright, also for the x-axis. This is confusing.
Sorry, something went wrong.
There was a problem hiding this comment.
Please open a new issue with the details of what you are seeing.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, done, see #28772
Sorry, something went wrong.
The public API is available since matplotlib 3.7 matplotlib/matplotlib#23692
| Back | FazBrowse Home | New Git URL |
PR Summary
Addresses #23603
PR Checklist
Tests and Styling
Documentation