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 pad #71

Merged
merged 4 commits into from
Dec 26, 2024
Merged

ENH: add pad #71

merged 4 commits into from
Dec 26, 2024

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Dec 26, 2024

closes gh-69

Add an np.pad replacement, as discussed at https://github.com/scipy/scipy/pull/22122/files#r1895558533

Copy link
Collaborator

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

Could you just add the array-agnostic path first in this PR? If we want to start adding dispatching to more efficient implementations for some libraries, we can consider adding that in a follow-up. I think I would rather dispatching code lived in a separate file to avoid cluttering the implementations.

There shouldn't be any need to use is_array_api_strict_namespace anywhere in this library.

@lucascolley lucascolley changed the title ENH: add pad ENH: add pad Dec 26, 2024
@lucascolley lucascolley added enhancement New feature or request new function labels Dec 26, 2024
@ev-br
Copy link
Contributor Author

ev-br commented Dec 26, 2024

  • np.pad is a thing
  • jax.numpy.pad is a thing
  • torch.nn.functional.pad is a thing
  • array_api_strict.pad is not
  • "generic" implementation needs mutable arrays, so is not going to work on jax.numpy

I don't understnd the request, and I though we covered this in the scipy issue.

@lucascolley
Copy link
Collaborator

"generic" implementation needs mutable arrays, so is not going to work on jax.numpy

Ah, okay. In which case, I think we should still just add the "generic" implementation here, then look at dispatching immediately after that. This is just one of the tricky situations we have to work with when array-api-strict is not strict enough to give us generic.

@lucascolley
Copy link
Collaborator

By the way, if you notice any improvements that we should make to https://data-apis.org/array-api-extra/contributing.html, would be great to hear.

@ev-br
Copy link
Contributor Author

ev-br commented Dec 26, 2024

I think we should still just add the "generic" implementation here, then look at dispatching immediately after that.

I still don't follow. This PR is I believe what scipy needs. What is wrong with the PR, what is the request exactly?

@lucascolley
Copy link
Collaborator

lucascolley commented Dec 26, 2024

What is wrong with the PR, what is the request exactly?

I'll push changes to show you what I mean. Currently, the other functions in this library do not use any delegation to specific array libraries. Since we want to add that in order to use this in SciPy, this PR is doing 2 things which can be quite cleanly split:

  1. add a pad which is similar to all of the other existing functions - array-agnostic, tested against array-api-strict.
  2. add delegation to existing libraries on top, perhaps also test with those libraries

My request was just to do (1) first here before we think about how delegation should work in array-api-extra.

@ev-br
Copy link
Contributor Author

ev-br commented Dec 26, 2024

Ah, so the request is to rewrite the history into two commits: one for the "generic" implementation and the other with branching on namespace? This I can do easily. Or, like you said, feel free to rewrite the history here to your liking.

@lucascolley
Copy link
Collaborator

Not exactly - I think there is potential to be smarter about how we do delegation, similar to what you and Matt have implemented in SciPy, if we are going to open up to the possibility of delegating for all functions in this library.

@ev-br
Copy link
Contributor Author

ev-br commented Dec 26, 2024

By the way, if you notice any improvements that we should make to https://data-apis.org/array-api-extra/contributing.html, would be great to hear.

It's great actually, thanks for the pointer.
This PR as is misses the "add to api_reference.md"; will add unless you beat me to it given #71 (comment)

@ev-br
Copy link
Contributor Author

ev-br commented Dec 26, 2024

I think there is potential to be smarter about how we do delegation, similar to what you and Matt have implemented in SciPy, if we are going to open up to the possibility of delegating for all functions in this library.

Okay.... I'd expect something like that is an overkill for a handful of if-elif-else branches in pure python code, but this is well in the "tastes differ" territory. Am rather curious to see what you have in mind.

Copy link
Collaborator

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

this LGTM now if you're happy with it! We can look at adding delegation on top next.

Copy link
Contributor Author

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

LGTM as an intermediate step, given that it's not usable on jax and suboptimal on other backends.
If we're adding more immediately after, updating the docs can be done in that immediately after.

@lucascolley lucascolley merged commit 169f21d into data-apis:main Dec 26, 2024
9 checks passed
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.

ENH: add pad
2 participants