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

Add support for ax.locator_params() #126

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sjsmith757
Copy link

@sjsmith757 sjsmith757 commented Apr 5, 2023

🚀 Pull Request

Description

Added the set_params method to the NetCDFTimeDateLocator class to support updating the number of ticks after the nc-time-axis is created. This allows for the locator parameters of the underlying AutoLocator to be modified. A full list of available parameters is here.

This can be done using plt.locator_params(axis='x') as the simplest or plt.gca().xaxis.get_major_locator().set_params() as a more fine-tuned alternative, assuming the time axis is the x axis.

Fixes #20

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 25.00% and project coverage change: -1.32 ⚠️

Comparison is base (7e9acbd) 93.10% compared to head (84310b1) 91.78%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
- Coverage   93.10%   91.78%   -1.32%     
==========================================
  Files           1        1              
  Lines         203      207       +4     
  Branches       47       47              
==========================================
+ Hits          189      190       +1     
- Misses          7       10       +3     
  Partials        7        7              
Impacted Files Coverage Δ
nc_time_axis/__init__.py 91.78% <25.00%> (-1.32%) ⬇️

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@spencerkclark
Copy link
Member

Nice, thanks @sjsmith757 -- to eventually merge this we'll probably want to add a test or two of this new functionality. No worries though if you're too busy to look into that at the moment.

@sjsmith757
Copy link
Author

Hi @spencerkclark - I've added some unit tests for my new method, as well as one integration test to ensure users can call it simply using the pyplot interface and that the functionality works in the larger context. I think using the plt.locator_params() syntax is the optimal case for end users, so I didn't include a docstring for the new method (it's essentially just a pass-through method anyways). Let me know if you need anything else for this enhancement!

Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

Thanks @sjsmith757 -- looks great -- just a few really minor suggestions.

It actually would be nice if you could add a one-line docstring for set_params since it will show up in the documentation of the NetCDFTimeDateLocator class (see the table of methods here). Maybe it could link to the MaxNLocator documentation page via :py:class:`matplotlib.ticker.MaxNLocator` as a source for the possible keyword arguments to pass, since this otherwise might be tricky to track down.

Finally, please give yourself credit with a note in the latest "New Features" section of the Release Notes.

def set_params(self, **kwargs):
self._max_n_locator_days.set_params(**kwargs)
self._max_n_locator.set_params(**kwargs)
return
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return

Comment on lines +223 to +230
# Add more assertions to test the behavior of the method
# For example, you can assert that the internal state of the locator is updated correctly
self.assertEqual(
getattr(locator._max_n_locator_days, f"_{key}"), value
)
self.assertEqual(
getattr(locator._max_n_locator, f"_{key}"), value
)
Copy link
Member

Choose a reason for hiding this comment

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

I could go either way on including these checks. They are a nice explicit way of checking that the set_params method worked as expected, though they depend on private attributes of the MaxNLocator class. I'm not sure if anyone else has opinions.

@ESadek-MO ESadek-MO removed the request for review from stephenworsley August 30, 2023 09:24
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Control max number of ticks
4 participants