Skip to content

DOC: add explanation of dtype to parameter values for np.append - #26303

Merged
mattip merged 6 commits into
numpy:mainfrom
tuhinsharma121:np-append-documentation-enhancement
Apr 22, 2024
Merged

mattip merged 6 commits into
numpy:mainfrom
tuhinsharma121:np-append-documentation-enhancement

Conversation

@tuhinsharma121

Copy link
Copy Markdown
Contributor

Improve doc strings to include explanation of dtype to parameter values for np.append

closes #26291

@tuhinsharma121

Copy link
Copy Markdown
Contributor Author

@rkern @seberg I took a first stab at it. I need your guidance here.

@tuhinsharma121

Copy link
Copy Markdown
Contributor Author

install pkg-config is failing. I dont think its happening due to my code changes.

@tuhinsharma121
tuhinsharma121 force-pushed the np-append-documentation-enhancement branch from 2a7c235 to 1807e08 Compare April 18, 2024 09:22
@tuhinsharma121

Copy link
Copy Markdown
Contributor Author

all right. tests have passed now.

@tuhinsharma121

Copy link
Copy Markdown
Contributor Author

This PR is ready for review

cc @rkern @seberg

Comment thread numpy/lib/_function_base_impl.py Outdated
Comment on lines +5718 to +5719
flattened before use. If dtype of values is different from the
dtype of arr then their dtypes are compared to figure out the

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
flattened before use. If dtype of values is different from the
dtype of arr then their dtypes are compared to figure out the
flattened before use. If dtype of `values` is different from the
dtype of `arr` then their dtypes are compared to figure out the

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed now.

@mattip

mattip commented Apr 19, 2024

Copy link
Copy Markdown
Member

Please add [skip azp][skip cirrus][skip actions] to the commit message to only run the circleci documentation CI job.

@tuhinsharma121

Copy link
Copy Markdown
Contributor Author

Please add [skip azp][skip cirrus][skip actions] to the commit message to only run the circleci documentation CI job.

Thank you for pointing. Done.

@tuhinsharma121

Copy link
Copy Markdown
Contributor Author

@mattip Please let me know if any more changes need to be done.

@mattip

mattip commented Apr 21, 2024

Copy link
Copy Markdown
Member

I think this does not actually address the issue that prompted the PR, which was that calling np.append(np.array([1]), np.array([])) will result in a float64 array, not an int one. The change here makes sense, but could be added to pretty much every function that mixes two ndarrays. The special thing in the issue is that an empty array will have dtype=float64. Can you think of a way to document that special case? Maybe adding a Note, or an Example?

@tuhinsharma121
tuhinsharma121 marked this pull request as draft April 21, 2024 03:56
@tuhinsharma121
tuhinsharma121 marked this pull request as ready for review April 21, 2024 04:08

@mattip mattip left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few comments, what do you think?

Comment thread numpy/lib/_function_base_impl.py Outdated
>>> c.dtype
float64

Default dtype for empty lists is `float64` thus making the output of dtype

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move this comment to above the example

Suggested change
Default dtype for empty lists is `float64` thus making the output of dtype
Default dtype for empty ndarrays is `float64` thus making the output of dtype

Comment thread numpy/lib/_function_base_impl.py Outdated
Comment thread numpy/lib/_function_base_impl.py Outdated
@tuhinsharma121
tuhinsharma121 marked this pull request as draft April 22, 2024 05:37
@tuhinsharma121
tuhinsharma121 marked this pull request as ready for review April 22, 2024 05:47
@mattip
mattip merged commit 8a6ef43 into numpy:main Apr 22, 2024
@mattip

mattip commented Apr 22, 2024

Copy link
Copy Markdown
Member

Thanks @tuhinsharma121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DOC: np.append docs should explain how appending different dtypes works

2 participants