Add keyword numticks in LogLocator in _autolev in contour.py #20034
Conversation
| @@ -1088,7 +1088,7 @@ def _autolev(self, N): | |||
| """ | |||
| if self.locator is None: | |||
| if self.logscale: | |||
| self.locator = ticker.LogLocator() | |||
| self.locator = ticker.LogLocator(numticks=N) | |||
There was a problem hiding this comment.
Should this be N+1 like the line below?
The docstring of this function will also need to be changed (as it claims that this is not supporte).
There was a problem hiding this comment.
I think, purpose of N is to set the level to N , idk if it should be N+1 I would like to ask @jklymak to weigh in! Regarding docstring, I guess it should have been doing this thing based on N but yeah I can update the docstring, if you all would suggest that!
There was a problem hiding this comment.
Sorry, I don't have any particular input here about the "right" behaviour. I think you should just make it consistent with what LogLocator would do an an axes. You will either break some tests with this, or if there were no tests will need to create some tests that exercise this...
In the above the changes are quite large, so you need to argue that they are better, or more flexible somehow. Maybe the default needs to be something that is as backwards compatible as possible.
Overall, though, I think passing numticks through to the LogLocator makes sense, but you should argue it a bit more completely here to get buy-in.
![github-actions[bot]](https://web.archive.org/web/20240110183404im_/https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.
|
@hp77-creator Are you still interested in working on this? I am going move this to the "future release" milestone for now. |
|
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
github-actions
bot
added
the
status: inactive
|
@tacaswell is this feature merged in any other PR? |
github-actions
bot
removed
the
status: inactive
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).Affects #19856
Made changes in the
_autolev()method to includenumtickskeyword argument in theLogLocatorcalled inside the function. After the changes, following images are obtainedwhich earlier used to be like this
