-
Notifications
You must be signed in to change notification settings - Fork 89
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
C++ refactoring: unique and is_unique #1111
Conversation
In PR #1117 (which will auto-merge soon), all v2 tests are being moved into a new The other directory restructuring that I'll be doing today will be to copy high-level code into |
Codecov Report
|
@jpivarski - there are a couple of places where for loops should be replaced by newly written kernel functions - they are marked with FIXME. Apart from that I think, it's done. |
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.
Great!
I know this is a lot of work, and I see your FIXMEs of the last issues to fix before it's done. I added to that some choices of exception class: C++ without access to pybind forced us to put a lot of different types of exceptions into only one class, ValueError (std::invalid_argument
in C++), but some of the ones labeled below should instead be np.AxisError
. We also want to maintain the distinction between user errors (ValueError, TypeError, np.AxisError
) and internal programming errors, which are AssertionErrors (RuntimeErrors in Awkward 1, as this is what std::runtime_error
produced, and that was our closest C++ equivalent).
One thing that I didn't mention below is to add type-tracer tests after each value test, which I also mentioned to @stormiestsin in #1135 (comment). They can usually be added in a semi-automated way, and they ensure that the code is Dask-ready.
Other than that, thanks for all of this; I can see the intricacy of a lot of these cases!
No description provided.