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

ENH: add kron and expand_dims #7

Merged
merged 10 commits into from
Oct 23, 2024
Merged

Conversation

lucascolley
Copy link
Collaborator

@lucascolley lucascolley commented Sep 24, 2024

@lucascolley lucascolley added enhancement New feature or request new function labels Sep 24, 2024
@lucascolley lucascolley marked this pull request as ready for review September 24, 2024 16:17
@lucascolley
Copy link
Collaborator Author

lucascolley commented Sep 24, 2024

@mdhaber I've taken your implementations from scipy/scipy#20077 (comment), made some modifications for edge-cases, and added type annotations as well as xp-compatible tests and docs based on NumPy's. Any feedback appreciated if you'd like to take a look!

@lucascolley
Copy link
Collaborator Author

@izaid

I've used kron. It's valuable. I'd have to double-check if it can be implemented just with array operations (probably can), and if it can I think it should be.

Indeed it can!

@mdhaber
Copy link
Contributor

mdhaber commented Sep 24, 2024

I was just zombie-translating from NumPy to array API with that, so I'm afraid I don't have any insight here.

@izaid
Copy link

izaid commented Sep 24, 2024

Is this expand_dims for a tuple of axes?!?!?!? You have just made my day.

I still think this should be in array API properly, but happy for it to live here until / if that happens.

@lucascolley
Copy link
Collaborator Author

lucascolley commented Sep 24, 2024

Is this expand_dims for a tuple of axes?!?!?!? You have just made my day.

yes 😁

image

It basically just sorts the tuple (my fix) and applies xp.expand_dims for each item (Matt's idea).

@izaid
Copy link

izaid commented Sep 24, 2024

It basically just sorts the tuple (my fix) and applies xp.expand_dims for each item (Matt's idea).

Fantastic!!! I specifically asked for this change to the array API (going from supporting a single axis to a tuple of axes), but it hasn't happened yet. See: data-apis/array-api#760

I use such a thing literally all the time.

Edit: Also, some of the existing libraries (NumPy and CuPy, but not PyTorch) do support expand_dims with a tuple of axes already. Maybe you want to dispatch directly to them in those cases?

@lucascolley
Copy link
Collaborator Author

aha, reading @asmeurer's comments in that issue I see the concern for ambiguity now. This implementation chooses to always add the same number of dimensions as the length of the tuple, and resolves negatives by taking the positions modulo the number of dimensions in the output. I should check that this matches what NumPy does, and add a note to the docs.

Edit: Also, some of the existing libraries (NumPy and CuPy, but not PyTorch) do support expand_dims with a tuple of axes already. Maybe you want to dispatch directly to them in those cases?

My intention with this package is to target just the standard - it isn't concerned with which real-world implementations exist (other than array-api-strict for testing).

It's quite simple to write small wrappers which delegate to known array libraries around array-agnostic functions like this, but I would rather keep that separate for now. Perhaps a separate package could do that and this will just be a backend for that, or perhaps that will take over this package eventually. I would rather that consumer libraries deal with which array libraries they would like to delegate to in their own code for now.

@izaid
Copy link

izaid commented Sep 24, 2024

aha, reading @asmeurer's comments in that issue I see the concern for ambiguity now. This implementation chooses to always add the same number of dimensions as the length of the tuple, and resolves negatives by taking the positions modulo the number of dimensions in the output. I should check that this matches what NumPy does, and add a note to the docs.

Edit: Also, some of the existing libraries (NumPy and CuPy, but not PyTorch) do support expand_dims with a tuple of axes already. Maybe you want to dispatch directly to them in those cases?

My intention with this package is to target just the standard - it isn't concerned with which real-world implementations exist (other than array-api-strict for testing).

It's quite simple to write small wrappers which delegate to known array libraries around array-agnostic functions like this, but I would rather keep that separate for now. Perhaps a separate package could do that and this will just be a backend for that, or perhaps that will take over this package eventually. I would rather that consumer libraries deal with which array libraries they would like to delegate to in their own code for now.

Yeah, I get the idea for keeping it simple! Makes sense, just wanted to point it out if you weren't aware. I'm just happy to not have to maintain an expand_dims anymore.

@lucascolley
Copy link
Collaborator Author

This PR is ready from my side. I'll leave it open for a while for any comments.

I've documented the resolution to the ambiguity in data-apis/array-api#760, which is to follow NumPy in disallowing a tuple containing both positive and negative indices referring to the same position in the expanded shape.

@lucascolley
Copy link
Collaborator Author

lucascolley commented Sep 27, 2024

kron

Implementing a new function that is not in the standard.

  • Signatures match between NumPy, PyTorch and JAX (PyTorch has an out= kwarg too).
  • No particular design choices here, just following the existing implementations.
  • No limitations to address as far as I am aware.

expand_dims

Adding the ability for axis to be a tuple of ints on top of the standard.


@kgryte good to proceed?

@lucascolley
Copy link
Collaborator Author

Gentle bump here! If I don't hear back, I'll probably merge this soon and circle back before a new release is cut.

@lucascolley lucascolley merged commit be06b63 into data-apis:main Oct 23, 2024
7 checks passed
@lucascolley lucascolley deleted the kron branch October 23, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new function
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants