Skip to content

Conversation

@djkirkham
Copy link
Contributor

@djkirkham djkirkham commented Sep 8, 2016

Since the netcdftime.datetime object now has microsecond precision, some tests are failing due to rounding errors.

This change fixes the errors by introducing a function num2date_to_nearest_second, which attempts to replicate the old behaviour of netcdftime.num2date (although it accepts a cf_units.Unit rather than unit string and calendar arguments.)

I say 'attempts' because when num2date is given a value in seconds representing a time with a half second, it will round it up or down arbitrarily, and it is apparently impossible to replicate this behaviour with the new version. The new function will always round up.

@djkirkham djkirkham mentioned this pull request Sep 8, 2016
cube.add_aux_coord(dim_coord, coord_dim)


def _num2date_to_nearest_second(time_value, units):
Copy link
Member

Choose a reason for hiding this comment

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

I think that this function may prove to be useful for users experiencing issues with netcdf4 times

I think that it is worth considering making it public and adapting the first few comment lines as the docstring.

I would likely leave the note that this behaviour... as a developer comment and not include it in teh docstring in this case

@marqh
Copy link
Member

marqh commented Sep 8, 2016

this looks like a sensible and well crafted change. It deals with a set of known issues within netcdf4-python and should make Iris less fragile with respect to versions of netcdf4

the open question for me is the public/private nature of the iris.util function

@djkirkham
Copy link
Contributor Author

djkirkham commented Sep 8, 2016

the open question for me is the public/private nature of the iris.util function

Perhaps it could be added to the cf_units api, possibly as an optional argument to Unit.num2date

@marqh
Copy link
Member

marqh commented Sep 9, 2016

Perhaps it could be added to the cf_units api, possibly as an optional argument to Unit.num2date

I like this apporach, I think that num <> date is already in scope for cf_units

@bjlittle @pelson would you support this approach?
Is it useful for @djkirkham to propose such a change to cf_units for us to see and him to test?

@djkirkham
Copy link
Contributor Author

djkirkham commented Sep 15, 2016

The change has been made to cf_units (SciTools/cf-units#66) so this PR is no longer needed.

@djkirkham djkirkham closed this Sep 15, 2016
@djkirkham djkirkham deleted the fix-datetime-rounding-issues branch October 26, 2017 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants