-
-
Notifications
You must be signed in to change notification settings - Fork 3k
WIP: compare datetimes and timedeltas with approx #4760
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
WIP: compare datetimes and timedeltas with approx #4760
Conversation
|
TODO: fix |
|
Thanks for taking a first pass at this. Here are a few comments off the top of my head:
|
|
How would you propose to define the tolerance for mixed-type containers? |
|
im under the impression we need to take a look at this more structurally - until we have a structure that brings together numeric and time data we should perhaps take a look at having the approximations for date/time under a different name as they require different behaviour |
|
Why does time require behavior that is more different than Decimal? I think they are similar in that the semantics of approx apply to them, and in that they require a tolerance specified in different units. The whole premise of my proposal is that time data can be compared numerically, under the same approx framework. If not, I'm not sure it's worthwhile. If we are ok with the current implementation of ApproxDecimal, what's wrong with a parallel implementation of ApproxDateTime? |
|
@brianmaissy the main issue i see that the behaviours wrt values of rel/abs are structurally different and im not seeing a unification there off the bat, if there was i'd happily integrate it |
|
Ok, never mind then |
|
@brianmaissy actually with a night of sleep i do see a possible unification the key idea is that unlike number, where we know sane defaults, a datetime in a nested structure must be turned into a approx explicitly, so a nested object would be declared like time related approximations would require a explicit cast in all cases and thus ambiguity of the nested case goes away |
|
i just noticed that i had gone with a display example of the extended api, |
This is a preliminary implementation of the feature suggested in #4736.
I wanted to get something down in code to further our ability to discuss API and implementation options. Even if we decide not to change any of the main points, I will go over the implementation again for style and documentation, improve the tests, update the docs, and write a changelog entry.
I implemented approximate comparison for both datetimes and timedeltas. In the current implementation at least, the same implementation pretty elegantly works for both.
Here are some issues that I though of while implementing it:
ApproxScalarexplaining that it is not supported.approx(expected, seconds=5), similar to the constructor of timedelta. But I think it obscures clarity and simplicity for a very marginal gain in conciseness.approx()isabs. The sort of annoying thing is thatabsis the third parameter toapprox(), so we can't pass the tolerance as a second parameter without the keywordabs=. I thought of playing games with the arguments in order to switch the order, but I think it would create more problems than it's worth.