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: adding casting option to numpy.stack. #21627

Merged
merged 18 commits into from Jun 8, 2022

Conversation

jhonatancunha
Copy link
Contributor

See #20959

np.concatenate and np.stack are similar methods, but only np.concatenate has the casting option.

This PR puts the casting option into the np.stack method to control what kind of data casting may occur

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks, great start. If we add casting=, maybe we should probably also add dtype= to match `concatenate.

You accidentally checked in a generated C file, might be missing from .gitignore... Please make sure to not include it.

Further we definitely need some basic tests here (pretty basic should be fine, since it just forwards to concatenate).

numpy/core/shape_base.py Outdated Show resolved Hide resolved
numpy/core/shape_base.pyi Outdated Show resolved Hide resolved
@BvB93 BvB93 added 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes 59 - Needs tests labels May 29, 2022
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

@jhonatancunha we still need basic tests and those style changes undone in the pyi file (even if they are auto-inserted maybe, my guess is the pep8 rules should not quite apply for stub files here).

Further, a very brief release note may be good (even a single bullet point), the instruction for those are in https://github.com/numpy/numpy/blob/main/doc/release/upcoming_changes/README.rst

The last thing I am wondering is if we should make sure to cover vstack, hstack etc. here, or just focus on stack since it is to some degree a preferred API probably.

numpy/core/shape_base.py Show resolved Hide resolved
jhonatancunha and others added 5 commits June 2, 2022 20:56
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
… method stack.

See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
@jhonatancunha jhonatancunha requested a review from seberg June 3, 2022 01:58
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me, the tests are probably even more thorough then necessary :).
The only thing might be polishing the release note a bit more, I think (we may also just do that as maintainers before merging though).

The one other thing is, that now that we started this, should we look into the other stacking functions as well? That would be a good followup (unless we want to explicitly not do that).

doc/release/upcoming_changes/21627.new_feature.rst Outdated Show resolved Hide resolved
doc/release/upcoming_changes/21627.new_feature.rst Outdated Show resolved Hide resolved
numpy/core/tests/test_shape_base.py Show resolved Hide resolved
numpy/core/shape_base.pyi Outdated Show resolved Hide resolved
numpy/core/shape_base.pyi Outdated Show resolved Hide resolved
jhonatancunha and others added 3 commits June 4, 2022 14:55
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
…umpy.hstack.

See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
…g keyword arguments.

See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
numpy/core/shape_base.pyi Outdated Show resolved Hide resolved
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
@jhonatancunha jhonatancunha requested a review from BvB93 June 6, 2022 17:49
) -> NDArray[Any]: ...
@overload
def stack(
arrays: Sequence[ArrayLike],
axis: SupportsIndex = ...,
out: _ArrayType = ...,
out: None = ...,
Copy link
Member

@BvB93 BvB93 Jun 6, 2022

Choose a reason for hiding this comment

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

Suggested change
out: None = ...,
out: _ArrayType = ...,

This specific out type has to be changed back here to its previous value.
All other annotation-related changes now look good to go, in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Point of clarification: I'm referring to the out type of this specific overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BvB93 sorry for that, we already pushed the changes for this error. Thanks for your patience.

See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: jhonatancunha <jhonatancunha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
@seberg seberg removed 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes 59 - Needs tests labels Jun 6, 2022
@jhonatancunha jhonatancunha requested a review from BvB93 June 6, 2022 22:29
Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @jhonatancunha! I'll wait for @seberg to merge though.

@@ -0,0 +1,16 @@
``casting`` and ``dtype`` keyword arguments for `numpy.stack`
-------------------------------------------------------------
The ``casting`` and ``dtype`` keyword arguments is now available for `numpy.stack`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The ``casting`` and ``dtype`` keyword arguments is now available for `numpy.stack`.
The ``casting`` and ``dtype`` keyword arguments are now available for `numpy.stack`.

Copy link
Member

Choose a reason for hiding this comment

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

(Same for all release notes below)

``casting`` and ``dtype`` keyword arguments for `numpy.stack`
-------------------------------------------------------------
The ``casting`` and ``dtype`` keyword arguments is now available for `numpy.stack`.
To use it, write ``np.stack(..., dtype=None, casting='same_kind')``.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To use it, write ``np.stack(..., dtype=None, casting='same_kind')``.
To use them, write ``np.stack(..., dtype=None, casting='same_kind')``.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @melissawm, we already pushed the commits with those changes!

jhonatancunha and others added 2 commits June 8, 2022 10:48
See numpy#20959

Co-authored-by: alescrocaro <alescrocaro@gmail.com>
Co-authored-by: JessePires <jesserocha@alunos.utfpr.edu.br>
Co-authored-by: patriarka <matheussantanapatriarca2019@outlook.com>
@seberg
Copy link
Member

seberg commented Jun 8, 2022

Agreed, thanks everyone and Melissa for the review, lets put this in!

A few final notes, which would be nice to follow up on, but it is OK if it doesn't happen.

  • We also have dstack (in a different file)!
  • There is also np.ma.stack (etc.). For me MA is in a weird state were we hope for a better replacement, but followups to fix it up would be great anyway.
  • Only if we follow-up with dstack: I think it might make sense to slim the release note down to a single heading and just list all of the stacking functions individually in the text.

@seberg seberg added the 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. label Jun 8, 2022
@seberg seberg merged commit 126046f into numpy:main Jun 8, 2022
BvB93 added a commit to BvB93/numpy that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 - Enhancement 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants