Skip to content

PR: Add meshgrid #145

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

Merged
merged 10 commits into from
Apr 19, 2021
Merged

PR: Add meshgrid #145

merged 10 commits into from
Apr 19, 2021

Conversation

steff456
Copy link
Member

@steff456 steff456 commented Mar 16, 2021

This PR

  • Adds specification for meshgrid, which returns coordinate matrices from coordinate vectors.

Notes

@steff456 steff456 requested review from rgommers and kgryte March 16, 2021 03:16
@steff456 steff456 self-assigned this Mar 16, 2021
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @steff456. Overall looks good, a few comments.

@rgommers rgommers added the API extension Adds new functions or objects to the API. label Mar 20, 2021
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

I'm thinking that it may not be obvious enough what this function does exactly. There's many functions where we don't have extensive notes, but in this case it seems justified to add the pseudo-code for indexing (like in the Notes section of the numpy.meshgrid docstring).

Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

@steff456 Thanks for working on this! In my review, I mainly suggested changes to ensure consistency with the rest of spec (formatting, language, etc). I also propose consolidating some of the notes and special cases into the parameter and return value descriptions directly. Let me know if you have any questions!

Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM

@rgommers rgommers force-pushed the main branch 3 times, most recently from 0607525 to 138e963 Compare April 19, 2021 20:25
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, let's get this in. Thanks @steff456 and @kgryte

@rgommers rgommers merged commit 0cbbfea into data-apis:main Apr 19, 2021
@kgryte kgryte deleted the add-meshgrid branch February 18, 2023 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Adds new functions or objects to the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants