Add combinations

[Jayd-1234]

Added preliminary version of combinations. It has been tested with different JaggedArray inputs, and was working as expected. The tests will be given shortly.

Things that are to be done:

• Add more case checking
• Add docstring describing it's usage
• Add support for other array types ( not only JaggedArray)

[jpivarski]

• Put the import awkward.array.table (fully qualified) inside the function so that there isn't a dependency order among array types. Also, use the fully qualified name throughout (awkward.array.table.Table rather than Table).
• Remove vectorized_parents. User-facing methods won't say how they're implemented: we would either replace regular parents with this or not. I've already done performance tests: regular parents is faster for Numpy, so we keep that. This vectorized algorithm would be the right thing to do on a GPU (a future awkward.gpu acceleration package), but not for regular Numpy.
• Remove return_indices as an argument; this method should be named aproduct and always return indexes (like amin and amax). There should be another method named product that calls this method and applies the indexes to get values. Moreover, I've established a style in which we spell it "indexes" (English allows both, so we have to pick one for better search-and-replace).
• Remove the writeable argument; it should inherit self._writeable.
• The first jagged array is self, so that this can be used as a method. It should be a red flag when a method never uses self. The second jagged array should be named other, a convention I've established. You only need to check the type of other (which would exclude the None case, no need for an explicit check).
• Minimize the introduction of unnecessary names. For instance NUMEVENTS (which, as a name, would be confusing outside of a physics context) is just len(self), which is short enough to use as an idiom. Moreover, it's only used once. Introducing names adds cognitive burden, making code harder to understand, especially when they're not meaningful names. Reducing the number of names would also help you avoid underscored_names.
• The dtype should be self.INDEXTYPE. It's centrally managed in the base class.
• Use spaces around arithmetic operations and after commas for clarity. (I use spaces around + and - but sometimes not * and / as a reminder of order of operations.)
• Inside a JaggedArray method, use the private member names: _content rather than content.

[jpivarski]

• Hide parents_from_offsets inside one of these methods (so that the user doesn't see it) or just inline the loop because it's only used in one place.
• Don't copy-paste huge blocks of code! product uses the output of argproduct, so have it call argproduct to extract what it needs, rather than reimplement it. Even the sanity checks (ValueErrors) don't have to be re-checked: if product calls argproduct, argproduct does all the necessary checks.
• Don't convert arrays with astype if you don't have to. numpy.arange takes a dtype argument, so use that.

[davidlange6]

should we check that counts1.shape == counts2.shape?

[Jayd-1234]

That won't be necessary, as we are doing shape checking for starts array. The shape of counts and starts are same.

[davidlange6]

add a small bit of documentation about what this function does.

[Jayd-1234]

Check it out. I have added simple docstrings describing the usage

[davidlange6]

content is needed in this function only to get the length of the output array. This is almost encoded into offsets. I wonder if a simpler solution could be used? Eg, to either pass in len(content) or to append the N+1 offset of the offsets array (which may be useful for other reasons)

[davidlange6]

(actually my second suggestion looks to be already the case - eg, can I not do

out=numpy.full(offsets[-1],-1, dtype=selfINDEXTYPE)

[Jayd-1234]

Yeah you are right! We need the length only. I will do it by tomorrow morning ( travelling now).

[Jayd-1234]

Done!

[jpivarski]

I had to make some corrections to the conventions (e.g. "offsets" we being named "counts") and fixed a bug (argproduct Table's length was set to pairs_indexes[-1], which is one too few). Also removed redundant implementations.

However, now that I've pushed to what I thought was this pull request, I don't see my commit showing up here. Is it on your fork?

[jpivarski]

Superseded by pull request #2 (which has all of these commits, plus one from me).

[Jayd-1234]

@jpivarski Thanks! That was a sneaky little bug..

I don't see the commit on my fork. Maybe it was closed?