8000 ENH: Add copy and device keyword to np.asanyarray to match np.asarray by JuliaPoo · Pull Request #26580 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH: Add copy and device keyword to np.asanyarray to match np.asarray #26580

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
Merged
merged 9 commits into from
Jun 6, 2024
Merged

ENH: Add copy and device keyword to np.asanyarray to match np.asarray #26580

merged 9 commits into from
Jun 6, 2024

Conversation

JuliaPoo
Copy link
Contributor

See #26196

@ngoldbaum
Copy link
Member

This needs a release note. It should be versionadded for 2.1 since I don't think it's an emergency to backport this for 2.0. Also needs new tests.

@ngoldbaum ngoldbaum added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label May 31, 2024
@JuliaPoo
Copy link
Contributor Author
JuliaPoo commented Jun 3, 2024 •
edited
Loading

I've added a release note as per suggested. Is there a way to flag this change for 2.1 in the release note? As for the tests, the underlying implementation for np.asanyarray (_array_fromobject_generic) is already covered by tests surrounding np.array, is there still a need to write tests? If so should I also add tests for np.asarray?

@JuliaPoo JuliaPoo closed this Jun 3, 2024
@JuliaPoo JuliaPoo reopened this Jun 3, 2024
@rgommers
Copy link
Member
rgommers commented Jun 3, 2024

Is there a way to flag this change for 2.1 in the release note?

That is automatic in the release notes; the tooling knows how to assemble the release notes for the next version from these snippets.

And you already used .. versionadded:: correctly in the docs (except there adding the correct version number manually is needed, and it should be 2.1.0 rather than 2.0.0).

As for the tests, the underlying implementation for np.asanyarray (_array_fromobject_generic) is already covered by tests surrounding np.array, is there still a need to write tests? If so should I also add tests for np.asarray?

Yes, this will be useful. Any public API construct should have at least a single direct test even when the underlying machinery is tested extensively in some other way. Probably TestDevice in _core/tests/test_multiarray.py is the place to put this. And yes, doing that for np.asarray too would be nice, good catch.

@JuliaPoo
Copy link
Contributor Author
JuliaPoo commented Jun 3, 2024

And you already used .. versionadded:: correctly in the docs (except there adding the correct version number manually is needed, and it should be 2.1.0 rather than 2.0.0).

Oh my bad, I completely forgot I did that, I've made the changes.

If the only new tests to add are related to the device keyword, then I'm not sure what tests to add since all methods discard the device keyword, and arr.device calls the following stub:

static PyObject *
array_device(PyArrayObject *self, void *NPY_UNUSED(ignored))
{
    return PyUnicode_FromString("cpu");
}

@rgommers
Copy link
Member
rgommers commented Jun 3, 2024

I think you only test that np.asanyarray(...), np.asanyarray(..., device=None) and np.asanyarray(..., device='cpu') all work and yield the same result - that ensures that usage of the keyword functions as expected. And that np.asanyarray(..., device='nonsense') raises an understandable exception.

@JuliaPoo
Copy link
Contributor Author
JuliaPoo commented Jun 4, 2024

Okay, I've added new tests

mattip
mattip reviewed Jun 5, 2024
View reviewed changes
Copy link
Member
@mattip mattip left a comment

Choose a reason for hiding this comment

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

LGTM. One typo and an optional vertical spacing suggestion

JuliaPoo and others added 3 commits June 6, 2024 09:13
@JuliaPoo @mattip
Update doc/release/upcoming_changes/26580.new_feature.rst
aeebbbe 
Co-authored-by: Matti Picus <matti.picus@gmail.com>
@JuliaPoo @mattip
Update numpy/_core/_add_newdocs.py
71eb077 
Co-authored-by: Matti Picus <matti.picus@gmail.com>
@JuliaPoo @mattip
Update numpy/_core/_add_newdocs.py
a9b773d 
Co-authored-by: Matti Picus <matti.picus@gmail.com>
@JuliaPoo
Copy link
Contributor Author
JuliaPoo commented Jun 6, 2024

Yea I think adding one more newline looks nicer too, Ive commited the suggestions

@mattip mattip merged commit e6ebcc7 into numpy:main Jun 6, 2024
67 of 68 checks passed
@mattip
Copy link
Member
mattip commented Jun 6, 2024

Thanks @JuliaPoo

@rgommers rgommers added this to the 2.1.0 release milestone Jun 10, 2024
@rgommers rgommers mentioned this pull request Jun 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattip mattip mattip left review comments

Assignees
No one assigned
Labels
01 - Enhancement 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes
Projects
None yet
Milestone
2.1.0 release
Development

Successfully merging this pull request may close these issues.
4 participants
@JuliaPoo @ngoldbaum @rgommers @mattip
0