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

Closes #3699 zeros, ones, full to return Array #3701

Merged

Conversation

ajpotts
Copy link
Contributor

@ajpotts ajpotts commented Aug 28, 2024

This changes the functions zeros, ones, and full in pdarrayclass to return Array type in the multi dimensional case. It also adds some extra unit tests.

Closes #3699 zeros, ones, full to return Array

@ajpotts ajpotts marked this pull request as ready for review August 28, 2024 01:17
@ajpotts ajpotts force-pushed the 3699-zeros-ones-full-to-return-Array branch from a1b3e70 to 817f357 Compare August 28, 2024 13:35
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing the motivation for importing the array-api module into arkouda propper to implement the multi-dim stuff. I would have thought that doing things the other way around would make more sense (i.e., make arkouda-propper support multi-dim pdarrays natively, and then build the Array API module on top of that (or, leave the Array API module as a separate thing for now)).

My mental model for the array-api module was for Array to be a thin wrapper around a pdarray that implements the Array API specification, while pdarray itself could implement something more like numpy's API. (Array needed to be a separate type, because there are places where the Array API deviates from numpy's API.)

I left a couple of comments about this, but haven't gone over all the changes yet:

arkouda/pdarraycreation.py Outdated Show resolved Hide resolved
arkouda/pdarraycreation.py Show resolved Hide resolved
@ajpotts ajpotts force-pushed the 3699-zeros-ones-full-to-return-Array branch 2 times, most recently from 81ee3df to 2da970b Compare August 28, 2024 16:21
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Contributor

@jaketrookman jaketrookman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! only comment is on a duplicated helper function

arkouda/pdarraycreation.py Outdated Show resolved Hide resolved
@ajpotts ajpotts force-pushed the 3699-zeros-ones-full-to-return-Array branch from 781866e to ce64bac Compare August 28, 2024 20:32
@stress-tess stress-tess added this pull request to the merge queue Aug 28, 2024
Merged via the queue into Bears-R-Us:master with commit 78633dd Aug 28, 2024
10 checks passed
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.

zeros, ones, full to return Array
4 participants