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.

Sign up for GitHub

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

Jump to bottom

*::flatten and *::count for axis != 0 #51

Closed
jpivarski opened this issue Jan 14, 2020 · 6 comments · Fixed by #152
Closed

*::flatten and *::count for axis != 0 #51

jpivarski opened this issue Jan 14, 2020 · 6 comments · Fixed by #152
Assignees
@ianna
Labels
feature New feature or request
Milestone
Minimum viable pr...

Comments

@jpivarski
Copy link
Member

All array types, including IndexedArray (and UnionArray, if it exists when you're ready for it).

Discussion of how to interpret negative axis here.
@jpivarski jpivarski added the feature New feature or request label Jan 14, 2020
@jpivarski
Copy link
Member Author

See #69 for further discussion of how to interpret negative axis.

@jpivarski jpivarski mentioned this issue Jan 14, 2020
Closed
jpivarski added a commit that referenced this issue Jan 15, 2020
@jpivarski
Issue #50: IndexedArray::flatten for axis=0 (#81)
428c3db 
Added `IndexedArray::flatten` for `axis=0` (other cases not covered, just as they're not covered for the other arrays, either).

Also added `*::count` (a special-case reducer) for `axis=0` because I thought I needed it. In fact, I also added a version that doesn't depend on `axis` and returns an `Index64` because I thought I need that, too. As it turns out, I was misinterpreting the intended output, and I didn't actually need either `count` method. However, it's probably going to be needed to implement `getitem` slicing for jagged arrays, so I'm leaving it in.

The `*::count` reducer should be extended to handle any `axis` value as part of issue #51. The handling is the same as it would be for `*::flatten`, so it should be abstracted out into a helper function.

* [WIP] Issue #50: IndexedArray::flatten for axis=0

* Set up arrays for testing IndexedArray::flatten.

* Implemented most of IndexedArray::flatten, but IndexedOptionArray case is wrong.

* Implemented *::count so that it will be possible to implement IndexedOptionArray, but the latter isn't done yet.

* Tests for *::count.

* Fix 32-bit.

* Need to implement toindex64 to implement IndexedOptionArray::flatten.

* [skip ci] toindex64 isn't going to work.

* Replace toindex64 with count64 (specifically what we need right now).

* Finished IndexedArray::count64.

* [skip ci] Working on IndexedOptionArray::flatten (finally).

* IndexedOptionArray::flatten is done (and it didn't need counts, after all).

* Update signatures.

* It was missing some index types.
@jpivarski
Copy link
Member Author

The axis parameter for count (a special-case reducer introduced in PR #81) should be handled in the same way as the axis parameter for flatten. This issue (#51) should add axis > 0 and axis < 0 cases to both flatten and count.

@jpivarski jpivarski changed the title *::flatten for axis != 0 *::flatten and *::count for axis != 0 Jan 15, 2020
@jpivarski
Copy link
Member Author

@ianna I added a lot of issues yesterday, which cover the gap from the present to the minimum viable product for users that is expected at the end of the 6-month sprint. (Every item in the README checklist for the 6-month sprint has been translated into an issue.)

I've assigned the array operations to you (flatten and count being the first two) and the new array types, Python front-end, and Numba implementation to me. That's completely negotiable.

If you're wondering where to get started, it's this one (issue #51), which we discussed before. Looking ahead to operations like the reducers in #69 has consequences for how negative axis should be interpreted, so it would be good to read ahead about what's coming.

@ianna
Copy link
Collaborator

ianna commented Jan 15, 2020

@ianna I added a lot of issues yesterday, which cover the gap from the present to the minimum viable product for users that is expected at the end of the 6-month sprint. (Every item in the README checklist for the 6-month sprint has been translated into an issue.)

I've assigned the array operations to you (flatten and count being the first two) and the new array types, Python front-end, and Numba implementation to me. That's completely negotiable.

If you're wondering where to get started, it's this one (issue #51), which we discussed before. Looking ahead to operations like the reducers in #69 has consequences for how negative axis should be interpreted, so it would be good to read ahead about what's coming.

Thanks, @jpivarski - Excellent! I thought it was a plan for the end of February :-)

I'm done with NumpyArray for axis != 0, not yet axis == -1. I need to read the issues for the latter.
I am adding more extensive tests and will open a PR either later today or tomorrow morning.

@jpivarski
Copy link
Member Author

Oh, good! With your work on NumpyArray, however, I put in some corrections to your initial implementation (PR #45) as part of adding IndexedArray::flatten yesterday (PR #81). Specifically, this:

https://github.com/scikit-hep/awkward-1.0/blob/428c3db9745af34798f3f970ffa48e2db009485b/src/libawkward/array/NumpyArray.cpp#L628-L634

It's a potential merging hazard!

@ianna
Copy link
Collaborator

ianna commented Jan 15, 2020

Oh, good! With your work on NumpyArray, however, I put in some corrections to your initial implementation (PR #45) as part of adding IndexedArray::flatten yesterday (PR #81). Specifically, this:

https://github.com/scikit-hep/awkward-1.0/blob/428c3db9745af34798f3f970ffa48e2db009485b/src/libawkward/array/NumpyArray.cpp#L628-L634

It's a potential merging hazard!

No problem, I'll start with a clean area.

@ianna ianna mentioned this issue Jan 16, 2020
Merged
@ianna ianna mentioned this issue Jan 29, 2020
Merged
@jpivarski jpivarski mentioned this issue Feb 15, 2020
Closed
@jpivarski jpivarski linked a pull request Mar 6, 2020 that will close this issue
Finish the count/sizes/num operation and the flatten operation #152
Merged
ianna pushed a commit to ianna/awkward that referenced this issue Sep 10, 2024
@matthewfeickert @jpivarski
build: Add lower bound on awkward of v2.5.0 (scikit-hep#51)
a78c3af 
* build: Add lower bound on awkward of v2.5.0

* The lower bound is empirically determined with

```
uv pip install --upgrade --resolution lowest-direct . && pytest tests/
```

as v2.5.0 requires awkward-cpp v26+ which is required for attrs to be
accessed.
   - c.f. https://github.com/scikit-hep/awkward/releases/tag/awkward-cpp-26

* ci: Add lower bound requirements tests

* Use 'uv pip install --upgrade --resolution lowest-direct' to
  automatically evaluate if the lower bounds of the dependencies
  specified are correct.

* replace cron-based scheduling with PRs and pushes to main

---------

Co-authored-by: Jim Pivarski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ianna ianna

Labels
feature New feature or request
Projects
None yet
Milestone
Minimum viable product for analysis
Development

Successfully merging a pull request may close this issue.

Finish the count/sizes/num operation and the flatten operation scikit-hep/awkward
2 participants
@ianna @jpivarski