Remove year 0 ticks if generated

[jbusecke]

Towards fixing #44.

@spencerkclark I was not sure how to add tests for this. Is there an example that you could point me to?

[coveralls]

Coverage increased (+1.3%) to 92.486% when pulling de5cb17 on jbusecke:patch-1 into 531dd0d on SciTools:master.

[spencerkclark]

Disclaimer: I'm not a maintainer of this package so take my suggestion with a grain of salt :). @trexfeathers was kind enough to review my recent (closed) PR; perhaps he'd be willing to weigh in here.

My two cents: if maintainers agree with this fix, I think adding some unit tests of NetCDFTimeDateLocator.tick_values that check this new behavior would make sense. You might be able to draw some inspiration from the existing test cases of this method:

nc-time-axis/nc_time_axis/tests/unit/test_NetCDFTimeDateLocator.py

Lines 81 to 115 in 531dd0d

	class Test_tick_values(unittest.TestCase):
	def setUp(self):
	self.date_unit = 'days since 2004-01-01 00:00'
	self.calendar = '365_day'
	def check(self, max_n_ticks, num1, num2):
	locator = NetCDFTimeDateLocator(max_n_ticks=max_n_ticks,
	calendar=self.calendar,
	date_unit=self.date_unit)
	return locator.tick_values(num1, num2)
	def test_secondly(self):
	np.testing.assert_array_almost_equal(
	self.check(4, 0, 0.0004),
	[0., 0.000116, 0.000231, 0.000347, 0.000463])
	def test_minutely(self):
	np.testing.assert_array_almost_equal(
	self.check(4, 1, 1.07), [1., 1.027778, 1.055556, 1.083333])
	def test_hourly(self):
	np.testing.assert_array_almost_equal(
	self.check(4, 2, 3), [2., 2.333333, 2.666667, 3.])
	def test_daily(self):
	np.testing.assert_array_equal(
	self.check(5, 0, 30), [0., 7., 14., 21., 28., 35.])
	def test_monthly(self):
	np.testing.assert_array_equal(
	self.check(4, 0, 365), [31., 120., 212., 304., 396.])
	def test_yearly(self):
	np.testing.assert_array_equal(
	self.check(5, 0, 5*365), [31., 485., 942., 1399., 1856.])

[jbusecke]

Ah sorry. I didn't do my research. Just associated you with the package. I'll take a look in a bit.

[jbusecke]

@trexfeathers are you generally ok with this fix? It is not a really sleek solution, but this problem has been popping up in many of my workflows: I work with long control runs of climate models which start in year 1, but the logic tends to try and 'pad' the axis limits a little bit and this cause a tick at year 0.

[trexfeathers]

@trexfeathers are you generally ok with this fix?

@jbusecke I only looked at nc-time-axis for the first time two weeks ago 😆, so I'd be uncomfortable passing judgement without doing some background reading first. I'm hopeful that I or another in my team will be permitted the time to give this the attention it deserves next week.

[trexfeathers]

@jbusecke it is also important that you sign the SciTools CLA (see the Governance notes for more detail), otherwise this PR cannot be merged even after it has been reviewed.

[jbusecke]

Thanks for the instructions. Just signed the form.

[trexfeathers]

OK @jbusecke, I've stared and prodded at this for a while and I'm confident that your methodology is the right one - the Matplotlib API for mticker.MaxNLocator() is such that there's no good way of forecasting what will be generated, especially with all the specific resolution handling that nc_time_axis does, so it's definitely best to go for a retrospective modification as you have.

---

As for testing, I suggest:

• Copying Test_tick_values.test_yearly(), to make a new test
  (Obviously if one of the other existing tests makes for a better starting template then feel free to use whatever is appropriate)
• Adapt the new test to generate an example of what you are trying to fix, and assert for a range of ticks that does not include the tick that your code should remove
• Give the new test a name that is descriptive of the scenario it is testing for

---

I've also commented with a specific suggested change to your proposed code. Thanks!

[jbusecke]

Hi @trexfeathers, super sorry for the long delay.

I have added a test, but I am quite unsure about the format (have only used pytest so far). My basic idea was to create ticks that would default to year 0 and loop over every calendar type. I also checked that none of the ticks has the .year 0, but it is probably even ok to just confirm that the test does not fail.

Happy to change this any way you see fit.

[trexfeathers]

@jbusecke thanks for persevering, especially having to use unittest 🤢 - we all want pytest too, when there's time!

And sorry for the gradual pace of this; I'm regularly pushing for more time to look at nc-time-axis.

I reckon you're nearly there, but have some suggestions for improved coverage and good setup practice.

[trexfeathers]

but it is probably even ok to just confirm that the test does not fail.

I had a similar thought, but the few extra lines we are working on make it much clearer what we're aiming for.

[jbusecke]

Ok I think I have implemented all the changes you requested (and reversed the automatic formatting introduced by my editor).
Let me know if there is anything else you would want me to change.

Many thanks for the patient review!

[trexfeathers]

Nice job, LGTM!