mods needed for cftime PR #202 #51
Conversation
|
commented out a test in test_NetCDFTimeDateLocator.py that removes year zero. Year zero in a reference date is now explicity checked for in cftime.datetime (an error is raised if it is used). |
There was a problem hiding this comment.
Hi @jswhit thanks for you vigilance, and sorry that the support for nc-time-axis is so sporadic. I will keep trying my best to carve out time when I can.
Having read around the issues discussed here, I've suggested some alternatives for dealing with them. Hopefully they sound sensible to you.
Also: please can you sign the SciTools CLA (see the Governance notes for more detail), otherwise this PR cannot be merged even after it has been reviewed.
| """ def test_yearly_yr0_remove(self): | ||
| for calendar in self.all_calendars: | ||
| # convert values to dates, check that none of them has year 0 | ||
| num2date = cftime.utime(self.date_unit, calendar).num2date | ||
| ticks = self.check(5, 0, 100 * 365, calendar) | ||
| year_ticks = [num2date(t).year for t in ticks] | ||
| if calendar in self.yr0_remove_calendars: | ||
| self.assertNotIn(0, year_ticks) | ||
| else: | ||
| self.assertIn(0, year_ticks) | ||
| """ |
There was a problem hiding this comment.
test_yearly_yr0_remove erroring is a symptom of a deeper problem that Unidata/cftime#202 has caused. The fix is a small one and would be safer than just commenting out an erroring test:
nc_time_axis.NetCDFTimeDateLocator.tick_values() relies on a non-calendar-aware instance of cftime.datetime on L156 and L165. Adding a calendar=None argument to both of these will allow tick_values() to function as before - it will remove invalid tick values where year=0 on certain calendars, rather than raising a ValueError.
|
CLA signed and comments addressed. |
|
@jswhit are you happy with my suggested change to |
|
@jswhit @trexfeathers, I made a PR to @jswhit's branch (jswhit#1) making @trexfeathers's suggested fix. Let me know if that looks good; it would be great to get this PR merged so that a new release of nc-time-axis can be issued (#54). |
|
Closed by #66 |
see discussion at Unidata/cftime#198
Small backwards compatibile change to one of the tests needed so that they pass with Unidata/cftime#202