Ensure dates are rounded to nearest second

[djkirkham]

Ensures num2date returns datetimes rounded to one second. Allowing microsecond precision introduces rounding errors.

[coveralls]

Coverage increased (+0.08%) to 89.724% when pulling 5924e09 on djkirkham:num2date-rounding into bd72437 on SciTools:master.

[DPeterK]

Out of interest, why has this been provided as a multi-line comment rather than a docstring?

[djkirkham]

This is just a comment for developers, so I don't think it should go in a docstring. I can provide a docstring based on L760-761 if you like, although this is a private function.

[coveralls]

Coverage increased (+0.08%) to 89.724% when pulling 85a4f4f on djkirkham:num2date-rounding into bd72437 on SciTools:master.

[coveralls]

Coverage increased (+0.08%) to 89.724% when pulling 85a4f4f on djkirkham:num2date-rounding into bd72437 on SciTools:master.

[djkirkham]

Hmm... previously the num2date functions would have worked when passing a list of values, but after my change they no longer do. It's not in the docstring, but I think I should make it work again. Should I change the doctring while I'm at it?

[DPeterK]

Should I change the doctring

Explicit is good 👍

[DPeterK]

Have you investigated whether you need to do the same for date2num? That is, has the accuracy of that function also been affected by the microseconds change?

[marqh]

I support this in principal.

I wonder whether we want to publicise this functionality and whether we want to enable access to microseconds

@djkirkham do you feel that implementing such changes would be simply additive, from the changes posted here?

[djkirkham]

@dkillick Good point. I hadn't looked into it, and it turns out that behaviour has changed. In the older version of netcdftime it seems date2num discards the microsecond component, but now it doesn't. I suppose we should make changes to keep this behaviour too.

[djkirkham]

@marqh

I wonder whether we want to publicise this functionality

Do you mean the functionality of being able to pass a list as argument?

do you feel that implementing such changes would be simply additive, from the changes posted here?

It should be simple to add an optional argument to keep the microseconds

[DPeterK]

I suppose we should make changes to keep this behaviour too.

We should indeed! And we should add a test that proves that the behaviour is correct, given that it's slipped through up to now.

[djkirkham]

Actually I'm not sure about including microseconds, since they're not available in the older version of netcdftime. Should this be included as a note in the docstring? What should happen if the user requests microseconds when using the older version? Raise an exception? Ignore it and return seconds resolution?

[marqh]

Actually I'm not sure about including microseconds, since they're not available in the older version of netcdftime. Should this be included as a note in the docstring? What should happen if the user requests microseconds when using the older version? Raise an exception? Ignore it and return seconds resolution?

This is a good point.

for now, I suggest we maintain the current behaviour, which explicitly ignores microseconds for all versions on netcdf4-python (this PR, as I understand it)

Changes to include these, to provide options to functions and to provide public access to private functions are easy to add at a later date

[DPeterK]

for now, I suggest we maintain the current behaviour

I agree. Banking this will be an excellent step forward and then we can take the time to think about the more complicated questions that @djkirkham raised.

[coveralls]

Coverage increased (+1.0%) to 90.611% when pulling a7018a1 on djkirkham:num2date-rounding into bd72437 on SciTools:master.

[marqh]

please make this a docstring

[coveralls]

Coverage increased (+0.9%) to 90.569% when pulling 0f19ad1 on djkirkham:num2date-rounding into bd72437 on SciTools:master.

[coveralls]

Coverage increased (+0.9%) to 90.541% when pulling 00297eb on djkirkham:num2date-rounding into bd72437 on SciTools:master.