Adopt microsecond precision in num2date and date2num

[rcomer]

🚀 Pull Request

Description

The _discard_microsecond and _num2date_to_nearest_second functions were introduced at #66 and ensure that we always work at second precision. The stated reason for this was

Allowing microsecond precision introduces rounding errors.

Since then, we have switched to using cftime under the hood (#85) and a quick search of the cftime repo suggests that they have worked on getting their functions working at microsecond precision (e.g. Unidata/cftime#187, Unidata/cftime#171). So it seems plausible that the rounding errors mentioned in #66 may no longer be a problem. This PR simply removes the microsecond handling from our functions to see which tests fail. The only failures are for tests that specifically use fractions of a second, and the values produced all look sensible to me:

Values produced within failing tests

test_discard_mircosecond

date = datetime.datetime(1970, 1, 1, 0, 0, 5, 750000)
cf_units.date2num(date, "seconds since 1970-01-01", "gregorian")

gives

5.75

test_fractional_second_*

for calendar in ['gregorian', '360_day', '365_day']:
    useconds = cf_units.Unit("seconds since 1970-01-01", calendar)
    print(useconds.num2date([0.25, 0.5, 0.75, 1.5, 2.5, 3.5, 4.5]))
    print('---')

gives

[cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 0, 250000, has_year_zero=False)
 cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 0, 500000, has_year_zero=False)
 cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 0, 750000, has_year_zero=False)
 cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 1, 500000, has_year_zero=False)
 cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 2, 500000, has_year_zero=False)
 cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 3, 500000, has_year_zero=False)
 cftime.DatetimeGregorian(1970, 1, 1, 0, 0, 4, 500000, has_year_zero=False)]
---
[cftime.Datetime360Day(1970, 1, 1, 0, 0, 0, 250000, has_year_zero=False)
 cftime.Datetime360Day(1970, 1, 1, 0, 0, 0, 500000, has_year_zero=False)
 cftime.Datetime360Day(1970, 1, 1, 0, 0, 0, 750000, has_year_zero=False)
 cftime.Datetime360Day(1970, 1, 1, 0, 0, 1, 500000, has_year_zero=False)
 cftime.Datetime360Day(1970, 1, 1, 0, 0, 2, 500000, has_year_zero=False)
 cftime.Datetime360Day(1970, 1, 1, 0, 0, 3, 500000, has_year_zero=False)
 cftime.Datetime360Day(1970, 1, 1, 0, 0, 4, 500000, has_year_zero=False)]
---
[cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 0, 250000, has_year_zero=True)
 cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 0, 500000, has_year_zero=True)
 cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 0, 750000, has_year_zero=True)
 cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 1, 500000, has_year_zero=True)
 cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 2, 500000, has_year_zero=True)
 cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 3, 500000, has_year_zero=True)
 cftime.DatetimeNoLeap(1970, 1, 1, 0, 0, 4, 500000, has_year_zero=True)]

So I can't see any evidence from the cf_units tests that rounding is still a problem.

If we were to strip out these functions and revert to cftime's microsecond precision, we would benefit from a simpler, easier to maintain codebase.

Thoughts?

[bjlittle]

@rcomer Let's rebase and respin.

I'm happy to then test this with iris to see if there are any side-effects.

Sound like a reasonable plan? 🤔

[rcomer]

Thanks @bjlittle I have rebased and also removed your retrospectively added tests - I forgot this branch predated those, but you just can't test what isn't there any more!

Remaining test failures appear to be as before i.e. expecting a whole number of seconds out when a fraction of seconds is passed in.

[rcomer]

I ran the Iris tests locally under py3.10 with this branch pip-installed. All passed ✅👍

[bjlittle]

@rcomer Okay, so this seems like a reasonable time to polish and bank this. I'm convinced that we should go for the removal of the rounding behaviour.

Since there is no downstream side-effects from testing with iris, that's enough for me.

I removed the decision required label, so go for it! 🚀

Simply show the PR some love, and extend the test coverage enough to convince ourselves that the cftime sub-second implementation is stable and I'll happily review and merge 👍

You happy to do that?

[rcomer]

OK I think this might be there. I'm a bit unclear where the tests should be but they are easy enough to block move if needed. Can confirm that Iris tests still pass with latest version of this branch.

[bjlittle]

@rcomer Awesome refactor.

Thanks for having the patience of a saint 👼