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

SegArray.__getitem__ to Server #1783

Closed
Ethan-DeBandi99 opened this issue Sep 14, 2022 · 3 comments · Fixed by #1790
Closed

SegArray.__getitem__ to Server #1783

Ethan-DeBandi99 opened this issue Sep 14, 2022 · 3 comments · Fixed by #1790
Assignees
Labels
enhancement New feature or request In Progress Work on ticket is in progress / ticket is actively being worked

Comments

@Ethan-DeBandi99
Copy link
Contributor

Part of #1682

Update SegArray.__getitem__() so that SegArray indexing is done server-side.

Note: There is a good bit of the structure all ready in place for this from SegString. We should be able to reuse this functionality.

@Ethan-DeBandi99 Ethan-DeBandi99 added the enhancement New feature or request label Sep 14, 2022
@Ethan-DeBandi99 Ethan-DeBandi99 self-assigned this Sep 14, 2022
@Ethan-DeBandi99
Copy link
Contributor Author

@reuster986 - I just ran across something that I wanted to get your take on. Initially, when I moved SegArray over to the server, I did not update the __init__ because I was worried it may interrupt user workflows. However, as I am moving functionality over it appears we will need to go to something in line with Strings in adding a class method from_parts with identical parameters to the existing SegArray.__init__ and then we could update the __init__ to build from the st.attrib call response method. I have a way to work around this, but it generates the SegArray on the server twice which I am not a fan of. What are your thoughts on update to a from_parts method to support the current constructor functionality?

@reuster986
Copy link
Collaborator

I'm not sure I understand all the pieces of this puzzle, but I think imitating the Strings implementation makes sense, since Strings are basically a special case of SegArray to begin with. In your proposal, would the user still be able to write sa = ak.SegArray(segs, vals) in Python? In other words, would the user ever need to call from_parts?

@Ethan-DeBandi99
Copy link
Contributor Author

Ethan-DeBandi99 commented Sep 15, 2022

The user would need to call ak.SegArray.from_parts(segs, vals). Strings doesn't do this because you can essentially just call ak.array(). The __init__ for strings builds of the return from the message. I just did a quick outline to verify, but the only things that updates is having to call from_parts. We could add a top level function that could be called like ak.array, something like ak.segarray(segs, vals) that would call from_parts behind the scenes.

@Ethan-DeBandi99 Ethan-DeBandi99 added the In Progress Work on ticket is in progress / ticket is actively being worked label Sep 16, 2022
stress-tess pushed a commit that referenced this issue Sep 22, 2022
* Update to SegArray to move indexing to the server. This also updates to use JSON return for SegArrays. Adds from parts function to build segarray.

Adds ak.segarray to build SegArray

* Cleanup after mypy and test failures

* Clean-up of unused functions

* updating benchmark

* Addressing Code review comments.

* Addressing review comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request In Progress Work on ticket is in progress / ticket is actively being worked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants