8000 ENH: Added ``fill`` option to ``np.atleast_2d`` and ``np.atleast_3d`` by eliegoudout · Pull Request #25170 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Added fill option to np.atleast_2d and np.atleast_3d #25170

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
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

ENH: Added fill option to np.atleast_2d and np.atleast_3d #25170

wants to merge 8 commits into from

Conversation

eliegoudout
Copy link
Contributor
@eliegoudout eliegoudout commented Nov 17, 2023 •
edited
Loading
  • Ready for review

Closes #25805 since it provides override option.

Added optional keyword argument fill to np.atleast_2d and np.atleast_3d, to specify where the missing dimension(s) should be added.

  • Default values are consistent with current behaviours,
  • I didn't add it to np.atleast_1d for obvious reasons, but if signature consistency is an issue, it can be done,
  • Implementation: I parse the non-default behaviours first, so that illegal values are mapped to default behaviour.

This can be useful when working with Fortan convention for example, or for locally-specifi reasons.

Illustrative examples:

>>> # 2D examples
>>> np.atleast_2d([1,2])  # default behaviour, fill='left'
array([[1, 2]])
>>> np.atleast_2d([1,2], fill='right')
array([[1],
       [2]])
>>>
>>> # 3D examples
>>> np.atleast_3d([1,2])  # default behaviour, fill='both'
array([[[1],
        [2]]])
>>> np.atleast_3d([1,2], fill='left')
array([[[1, 2]]])
>>> np.atleast_3d([1,2], fill='right')
array([[[1]],

       [[2]]])

@eliegoudout eliegoudout mentioned this pull request Nov 17, 2023
Closed
1 task
@eliegoudout eliegoudout marked this pull request as ready for review November 17, 2023 18:43
@eliegoudout
Copy link
Contributor Author

Can someone help me fix the failed build / give some feedback?
Thanks!

@charris charris changed the title ENH: Added fill option to np.atleast_2d and np.atleast_3d ENH: Added fill option to np.atleast_2d and np.atleast_3d Jan 13, 2024
@eliegoudout
Copy link
Contributor Author

Up

@eliegoudout
8000
Copy link
Contributor Author

Dear, @melissawm @charris, thank you both for having shown interest at some point. Do you think it is possible to get this PR reviewed or even discussed at some point? What would the procedure be? Because I do feel a bit unconfortable directly pinging you for attention. But I don't know any solution to get it out of the abyss!

Thanks!

@mattip mattip added the triage review Issue/PR to be discussed at the next triage meeting label Apr 11, 2024
@tylerjereddy
Copy link
Contributor

Somewhat related? data-apis/array-api#494

@mattip
Copy link
Member
mattip commented May 15, 2024

We discussed this in a recent triage meeting. Since there is no corresponding issue, could you explain the motivation? I did see #25805 about consistency with left-right and atleast_2d and atleast_3d, and #18386 about atleast_nd. Would it make more sense to add a atleast_nd with a kwarg, and leave these alone as legacy API?

@mattip mattip removed the triage review Issue/PR to be discussed at the next triage meeting label May 15, 2024
@eliegoudout
Copy link
Contributor Author
eliegoudout commented May 17, 2024 •
edited
Loading

We discussed this in a recent triage meeting. Since there is no corresponding issue, could you explain the motivation? I did see #25805 about consistency with left-right and atleast_2d and atleast_3d, and #18386 about atleast_nd. Would it make more sense to add a atleast_nd with a kwarg, and leave these alone as legacy API?

Thank you for your comment. I do think that atleast_nd is the right way to go ultimately. Regarding usage, I would personally propose something like

new_ary = np.atleast_nd(ary, nd, fill)

with

fill (str | Iterable[int]): Where to add missing dimensions, if any. If
    a string, it must be one of ``{'left', 'right'}``. Otherwise, it
    must be a permutation of ``np.arange(nd)``, in which case missing
    dimensions will be added in axes indexed by ``fill[:nd-ary.ndim]``.

This could be even more permissive in a few ways. For example fill could work with partial permutations, negative indexing, and mixture of indexing and left or right (playing a role somewhat similar to Ellipsis in other APIs). A few possibilities with nd=4:

fill=(0, -1, 1, 2)  # equivalent to (0, 3, 1, 2)
fill=(-1, 'left')  # equivalent to (3, 0, 1, 2)
fill=(0, 'right', 2)  # equivalent to (0, 3, 1, 2)

Also, still with nd=4 for the example, we could allow fill=(2,) or fill=2 and decide that it is either interpreted as (2, 'right') or (2, 'left') (depending on default behaviour choice), or as a constraint ary.ndim >= 3 (since there is no information provided by fill on where to add missing dimension if there is more than 1 missing).

The current implementations of atleast_2d and atleast_3d would be equivalent to

np.atleast_nd(ary, 2, fill='left')  # current `atleast_2d`
np.atleast_nd(ary, 3, fill=(2, 0, 1))  # current `atleast_3d`
np.atleast_nd(ary, 3, fill=(-1, 'left'))  # another possible syntax for current `atleast_3d`

For my PR, I only focused on a small modification, to avoid going through a long discussion / approval process required to add a new method. The use cases can be plenty. A simple one would be to allow a user to call a function treating 2d arrays by passing either one 2d array or a batch of 2d arrays (so a 3d tensor with batch_dim=0). The functino would then start off by using input = np.atleast_3d(input, 'left').

This can feel silly, but I'm pretty sure there numerous use cases, especially with atleast_nd as described above.

Thanks!

@eliegoudout
Copy link
Contributor Author

Any feedback?

I think it is reasonable to merge this as it is a very very light and compatible change, providing more flexibility.

As seen above, the implementation of atleast_nd has been mentioned a few times but not yet discussed much, so it might take some time to be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
01 - Enhancement
Projects
NumPy first-time contributor PRs
Status: Awaiting a code review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: Behavior of atleast_3d is surprising
3 participants
@eliegoudout @tylerjereddy @mattip
0