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
Conversation
There was a problem hiding this 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).
… into add_casting_to_stack
There was a problem hiding this 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.
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>
There was a problem hiding this 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).
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>
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
) -> NDArray[Any]: ... | ||
@overload | ||
def stack( | ||
arrays: Sequence[ArrayLike], | ||
axis: SupportsIndex = ..., | ||
out: _ArrayType = ..., | ||
out: None = ..., |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this 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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ``casting`` and ``dtype`` keyword arguments is now available for `numpy.stack`. | |
The ``casting`` and ``dtype`` keyword arguments are now available for `numpy.stack`. |
There was a problem hiding this comment.
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')``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To use it, write ``np.stack(..., dtype=None, casting='same_kind')``. | |
To use them, write ``np.stack(..., dtype=None, casting='same_kind')``. |
There was a problem hiding this comment.
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!
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>
… into add_casting_to_stack
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.
|
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