New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UTCDateTime fuzzy comparisons break sorting, max, min #1766

Closed
wants to merge 7 commits into
base: maintenance_1.1.x
from

Conversation

Projects
None yet
4 participants
@barsch
Member

barsch commented Apr 27, 2017

Closes #1765 (UTCDateTime fuzzy comparisons break sorting, max, min.)

@barsch barsch changed the title from test case and possible fix for #1765 to UTCDateTime fuzzy comparisons break sorting, max, min Apr 28, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer May 5, 2017

Member

Looks good to me - the remaining failing tests should be easy enough to sort out.

Member

krischer commented May 5, 2017

Looks good to me - the remaining failing tests should be easy enough to sort out.

@megies megies added .core bug labels May 8, 2017

@megies megies added this to the 1.1.0 milestone May 8, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jun 30, 2017

Member

There's some fails on this one.. any news on this @barsch?

Member

megies commented Jun 30, 2017

There's some fails on this one.. any news on this @barsch?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jul 27, 2017

Member

Punting to 1.1.1.. otherwise we'll never even get to 1.1.0

Everybody feel free to tackle it of course..

Member

megies commented Jul 27, 2017

Punting to 1.1.1.. otherwise we'll never even get to 1.1.0

Everybody feel free to tackle it of course..

@megies megies modified the milestones: 1.2.0, 1.1.0, 1.1.1 Jul 27, 2017

@megies megies changed the base branch from master to maintenance_1.1.x Nov 1, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 1, 2017

Member

Changed base branch to maintenance_1.1.x. Please ideally rebase on this base branch, eventually.

Member

megies commented Nov 1, 2017

Changed base branch to maintenance_1.1.x. Please ideally rebase on this base branch, eventually.

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 10, 2018

Member

Hey @barsch I just pushed some test cases, do you want me to dig into how to make them pass or did you already have a master plan?

Member

d-chambers commented Jan 10, 2018

Hey @barsch I just pushed some test cases, do you want me to dig into how to make them pass or did you already have a master plan?

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Jan 10, 2018

Member

nope - no master plan - feel free to tackle it ;)

Member

barsch commented Jan 10, 2018

nope - no master plan - feel free to tackle it ;)

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 10, 2018

Member

It looks like these lines aren't going to behave as expected due to floating point limitations.

a = round(self._ns / 1e9, self.__precision)
b = round(other._ns / 1e9, self.__precision)

for example

ns = 1_515_174_511_198_443_520
round(ns / 1e9, 6)  # returns 1515174511.198443 not 1515174511.198444
Member

d-chambers commented Jan 10, 2018

It looks like these lines aren't going to behave as expected due to floating point limitations.

a = round(self._ns / 1e9, self.__precision)
b = round(other._ns / 1e9, self.__precision)

for example

ns = 1_515_174_511_198_443_520
round(ns / 1e9, 6)  # returns 1515174511.198443 not 1515174511.198444
@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 11, 2018

Member

@barsch I just pushed a few changes, let me know what you think. All the UTCDateTime tests pass but there are lots of other (seemingly unrelated) failing tests that I will try to figure out later. We also need to rebase from current master before we can merge right?

Member

d-chambers commented Jan 11, 2018

@barsch I just pushed a few changes, let me know what you think. All the UTCDateTime tests pass but there are lots of other (seemingly unrelated) failing tests that I will try to figure out later. We also need to rebase from current master before we can merge right?

:param precision: optional, the required precision (<=9).
"""
_precision = self.precision if precision is None else precision
return round(round(self._ns, _precision - 9) / 1e9, _precision)

This comment has been minimized.

@barsch

barsch Jan 11, 2018

Member

I didn't even know this works: round(long, -3) - nice!

@barsch

barsch Jan 11, 2018

Member

I didn't even know this works: round(long, -3) - nice!

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Jan 11, 2018

Member

I don't know - I have the feeling the safest bet would be to stay with integer/long data type while comparing objects to an UTCDateTime object (by converting the other to nanoseconds with default precision) - as soon as we convert to float we will get precision issues ...

Member

barsch commented Jan 11, 2018

I don't know - I have the feeling the safest bet would be to stay with integer/long data type while comparing objects to an UTCDateTime object (by converting the other to nanoseconds with default precision) - as soon as we convert to float we will get precision issues ...

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Jan 11, 2018

Member

I think you are right. Best to leave it as a rounded int for comparison. I will pick this up again this weekend. Should I also attempt the rebase to current master? I am hoping that makes some of the other failures that seem unrelated go away.

I will also add a warning on setting attrs after init, thus paving the way to immutability in the future, if that is what we are going to do. Is there some kind of official process for making these sorts of decisions?

Member

d-chambers commented Jan 11, 2018

I think you are right. Best to leave it as a rounded int for comparison. I will pick this up again this weekend. Should I also attempt the rebase to current master? I am hoping that makes some of the other failures that seem unrelated go away.

I will also add a warning on setting attrs after init, thus paving the way to immutability in the future, if that is what we are going to do. Is there some kind of official process for making these sorts of decisions?

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Jan 11, 2018

Member

I'll be very happy if you take this PR over ;) - rebasing to master is needed - maybe even close this PR and open a new one if this is easier for you

immutability is afaik fine for most core developers, but maybe asking in Gitter gives some more input/thoughts on this matter

Member

barsch commented Jan 11, 2018

I'll be very happy if you take this PR over ;) - rebasing to master is needed - maybe even close this PR and open a new one if this is easier for you

immutability is afaik fine for most core developers, but maybe asking in Gitter gives some more input/thoughts on this matter

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Jan 16, 2018

Member

superseded by #2049

Member

barsch commented Jan 16, 2018

superseded by #2049

@barsch barsch closed this Jan 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment