-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: add 'dak.backend' and dak.to_list
and verify that they overload the ak.*
versions
#498
Conversation
Some floating point rounding error?
|
Either that or radically different contents somewhere in Only on Python 3.12 on MacOS... On a comparison that was already approximate... Maybe some binary change nudged it over the threshold. This is also marked as xfail with |
dak.to_list
and verify that they overload the ak.*
versions
Since the test failure doesn't seem to be reproduced, just running it again, here's its output, which we can revisit in the future if we see that failure again:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dak.to_list
implementation looks good! I can't check ✔️ approved! because I started this PR.
However, I'd say it's ready to be merged.
print(f"{ak.to_list(daa1.distance(daa2)) = }") | ||
print(f"{ak.to_list(caa1.distance(caa2)) = }") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was here to diagnose the test failure.
But now it looks like the test passed, just having been run a second time. (Usually, not a good thing!)
(we may come back to check on that array comparison, maybe floating point inaccuracy) |
|
||
Unlike most functions, this one requires a compute() of the data. | ||
""" | ||
return array.compute().to_list() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpivarski I wonder, should this be lazy and return a bag of lists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We chatted in the meeting and thought not - this is only likely to be used in testing for small data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if I called this function, I wouldn't be interested in lazy data. The use-case that came up here was that I wanted to debug a test failure.
The tests need type annotations! That's too much, man!