Unpin mpl #34
Unpin mpl #34
Conversation
| @@ -97,7 +97,7 @@ def __init__(self, max_n_ticks, calendar, date_unit, min_n_ticks=3): | |||
| self.min_n_ticks = min_n_ticks | |||
| self._max_n_locator = mticker.MaxNLocator(max_n_ticks, integer=True) | |||
| self._max_n_locator_days = mticker.MaxNLocator( | |||
| max_n_ticks, integer=True, steps=[1, 2, 4, 7, 14]) | |||
| max_n_ticks, integer=True, steps=[1, 2, 4, 7, 10]) | |||
There was a problem hiding this comment.
The docs mention that this should be between 1 and 10 but I'm not sure that is the ideal steps for nc-time-axis.
There was a problem hiding this comment.
Holy 💩 ... I've just joined the dots here!
During the marathon that was SciTools/iris#3019, I raised the issue SciTools/iris#3019 to capture the oddity of a documentation example graphical test that is raising a UserWarning from within mpl...
<snip>/lib/python2.7/site-packages/matplotlib/ticker.py:1853: UserWarning: Steps argument should be a sequence of numbers
increasing from 1 to 10, inclusive. Behavior with
values outside this range is undefined, and will
raise a ValueError in future versions of mpl.
After digging a little, it turns out that the plot in question
uses nc-time-axis to plot time on the xaxis... and the weird step values that are being passed into the matplotlib.ticker.MaxNLocator that cause it to raise the UserWarning are [1, 2, 4, 7, 14] no less!
Awesome! So nc-time-axis is indeed the root cause.
I'll play with the steps to see the impact on plots...
There was a problem hiding this comment.
It took me a while to figure that out just to find out that @QuLogic already solved this last year 😄
| @@ -108,7 +112,7 @@ def test_monthly(self): | |||
|
|
|||
| def test_yearly(self): | |||
| np.testing.assert_array_equal( | |||
| self.check(5, 0, 5*365), [31., 638., 1246., 1856.]) | |||
| self.check(5, 0, 5*365), [31., 485., 942., 1399., 1856.]) | |||
There was a problem hiding this comment.
With the change in the steps, and taking advantage of the classic style, the only test that fails is this one. I adapted the expected values to reflect the change but I'm not sure this is the ideal way to fix this. The README example gets 4 ticks instead of 3 with these changes but the figure looks good to me.
| import unittest | ||
|
|
||
| import cftime | ||
| import numpy as np | ||
|
|
||
| from nc_time_axis import NetCDFTimeConverter, CalendarDateTime | ||
|
|
||
| if sys.version_info[:2] == (2, 7): | ||
| unittest.TestCase.assertRaisesRegex = unittest.TestCase.assertRaisesRegexp | ||
|
|
There was a problem hiding this comment.
@ocefpaf Could we not just use from six import assertRaisesRegex instead?
We'd need to add it as an dependency...
There was a problem hiding this comment.
Sure. Let me update that.
| @@ -56,7 +60,7 @@ def test_nonequal_calendars(self): | |||
| unit = 'days since 2000-01-01' | |||
| val = [CalendarDateTime(cftime.datetime(2014, 8, 12), calendar_1), | |||
| CalendarDateTime(cftime.datetime(2014, 8, 13), calendar_2)] | |||
| with self.assertRaisesRegexp(ValueError, 'not all equal'): | |||
| with self.assertRaisesRegex(ValueError, 'not all equal'): | |||
There was a problem hiding this comment.
If you agree to using six then this will be
with assertRaisesRegex(self, ValueError, 'not all equal') :etc
setup.py
Outdated
| @@ -20,7 +20,7 @@ | |||
| author='Laura Dreyer, Philip Elson', | |||
| url='https://github.com/scitools/nc-time-axis', | |||
| packages=packages, | |||
| install_requires = ['matplotlib==1.*', | |||
| install_requires = ['matplotlib', | |||
There was a problem hiding this comment.
Not your doing, but let's put these in alphabetical order, thanks.
|
Fantastic! Thanks @ocefpaf. I'll cut a release with this in it shortly. |
Closes #23