Skip to content
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

Implement 'argcross' and 'cross'. #159

Merged
merged 11 commits into from Mar 11, 2020
Merged

Conversation

jpivarski
Copy link
Member

After some consideration, I'm going to use @nsmith-'s nifty trick: #78 (comment)

The reason against it was that although it uses broadcasting on the axis we want to cross, it might accidentally use broadcasting on some other axes as well. On the other hand, a pattern is emerging in which we're applying broadcasting to all functions of more than one array, so maybe that's a feature, not an antifeature.

Also, defining functions of more than one array in C++ is hard because the arrays might consist of different node types; checking all the possibilities is not scalable. There are a few such functions, such as Content::merge, but for these, we "bite the bullet" and do all the cases because it's a utility function used to simplify other functions (all that nasty complexity is concentrated in one place).

One difference, though: I'm not going to use empty slices (i.e. :) and np.newaxis (i.e. None) to create the RegularArrays; I'm going to put them in with awkward1._util.recursively_apply. This avoids the array-scaling overhead of tuple-slicing. If all the elements of the tuple-slice are : and np.newaxis, then no arrays need to be touched. Perhaps someday, Content::getitem will have enough optimizations that this would be the case, but as of right now, awkward1._util.recursively_apply is the only way to do it in O(1) time.

The cross function will not depend on argcross. In fact, argcross will have to be a little more expensive, as it needs to replace the data with integer arrays before crossing. Maybe that (equivalent to Awkward0's localindex) should be in C++.

@jpivarski jpivarski linked an issue Mar 11, 2020 that may be closed by this pull request
@jpivarski jpivarski changed the title [WIP] Implement 'argcross' and 'cross'. Implement 'argcross' and 'cross'. Mar 11, 2020
@jpivarski
Copy link
Member Author

In this world, argcross is defined in terms of cross! Everything here is done; when the tests pass, we merge and deploy.

@jpivarski jpivarski merged commit 97456d2 into master Mar 11, 2020
@jpivarski jpivarski deleted the feature/0078-argcross-and-cross branch March 19, 2020 20:28
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.

argcross and cross
1 participant