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.

Sign up for GitHub

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

Unpin mpl #34

Merged
merged 6 commits into from
May 31, 2018
Merged

Unpin mpl #34

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d41cc5
unpin mpl
ocefpaf May 28, 2018
bdccddd
steps goes from 1 to 10
ocefpaf May 28, 2018
5bce0d3
use classic to fix the last two tests
ocefpaf May 28, 2018
b12290e
fix DeprecationWarnings
ocefpaf May 28, 2018
8fcb5b0
deps in alphabetical order
ocefpaf May 30, 2018
4d9df15
use six
ocefpaf May 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nc_time_axis/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
aefec91c3601249cc9b3336dc4c8cdb31a64c6d997b3c0eccb5932d285e42f33
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...

Copy link
Member Author

Choose a reason for hiding this comment

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

It took me a while to figure that out just to find out that @QuLogic already solved this last year 😄

#23 (comment)

self.calendar = calendar
self.date_unit = date_unit
if not self.date_unit.lower().startswith('days since'):
Expand Down
10 changes: 7 additions & 3 deletions nc_time_axis/tests/unit/test_NetCDFTimeConverter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@
from __future__ import (absolute_import, division, print_function)
from six.moves import (filter, input, map, range, zip) # noqa

import sys
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

Copy link
Member

@bjlittle bjlittle May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@ocefpaf Could we not just use from six import assertRaisesRegex instead?

We'd need to add it as an dependency...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Let me update that.


class Test_axisinfo(unittest.TestCase):
def test_axis_default_limits(self):
Expand Down Expand Up @@ -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'):
Copy link
Member

Choose a reason for hiding this comment

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

If you agree to using six then this will be

with assertRaisesRegex(self, ValueError, 'not all equal') :

etc

NetCDFTimeConverter().default_units(val, None)


Expand Down Expand Up @@ -98,14 +102,14 @@ def test_non_cftime_datetime(self):
val = CalendarDateTime(4, '360_day')
msg = 'The datetime attribute of the CalendarDateTime object must ' \
'be of type `cftime.datetime`.'
with self.assertRaisesRegexp(ValueError, msg):
with self.assertRaisesRegex(ValueError, msg):
result = NetCDFTimeConverter().convert(val, None, None)

def test_non_CalendarDateTime(self):
val = cftime.datetime(1988, 5, 6)
msg = 'The values must be numbers or instances of ' \
'"nc_time_axis.CalendarDateTime".'
with self.assertRaisesRegexp(ValueError, msg):
with self.assertRaisesRegex(ValueError, msg):
result = NetCDFTimeConverter().convert(val, None, None)


Expand Down
6 changes: 5 additions & 1 deletion nc_time_axis/tests/unit/test_NetCDFTimeDateLocator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@

import unittest

import matplotlib.style
import matplotlib.dates as mdates
import cftime
import numpy as np

from nc_time_axis import NetCDFTimeDateLocator


matplotlib.style.use('classic')


class Test_compute_resolution(unittest.TestCase):
def setUp(self):
self.date_unit = 'days since 2004-01-01 00:00'
Expand Down Expand Up @@ -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.])
Copy link
Member Author

Choose a reason for hiding this comment

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

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.



if __name__ == "__main__":
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

Not your doing, but let's put these in alphabetical order, thanks.

'cftime',
'numpy',
'six'],
Expand Down