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

Pull xarray's nbytes from nbytes attribute on arrays #6797

Merged
merged 10 commits into from
Jul 22, 2022

Conversation

maxrjones
Copy link
Contributor

@maxrjones maxrjones commented Jul 16, 2022

This implements the suggestion in #4842 (comment) to leave the nbytes calculation to the backend array.

Also removed the duplicate docs entry reported in #6565

@maxrjones
Copy link
Contributor Author

Please let me know if I should add a docstring as suggested in #6565 (comment). I didn't yet because most of the properties do not have docstrings.

@dcherian
Copy link
Contributor

I should add a docstring as suggested

This would be great!

Adding docstrings for the other properties would be a great (separate) PR!

xarray/core/variable.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

Thanks @maxrjones can you push any WIP tests you might have for this?

@maxrjones
Copy link
Contributor Author

Thanks @maxrjones can you push any WIP tests you might have for this?

If it's alright I will wait for #6804 to be merged first because that will greatly simplify testing the changes. After that PR, the following could be added to xarray/tests/test_array_api.py to test both cases:

def test_properties(arrays) -> None:
    np_arr, xp_arr = arrays
    assert np_arr.nbytes == 48
    assert xp_arr.nbytes == 48

@dcherian
Copy link
Contributor

Ah great idea!

@TomNicholas
Copy link
Member

Great first contribution @maxrjones ! #6804 should auto-merge in a sec so you can then fetch and merge in main to write your tests 😄

xarray/tests/test_array_api.py Outdated Show resolved Hide resolved
xarray/core/variable.py Outdated Show resolved Hide resolved
@dcherian
Copy link
Contributor

Thanks @maxrjones I added a sparse variable test that fails on main.

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Thanks @maxrjones This is a great and impactful PR! Welcome to Xarray

@dcherian dcherian enabled auto-merge (squash) July 22, 2022 16:51
@dcherian dcherian merged commit 60f8c3d into pydata:main Jul 22, 2022
@maxrjones maxrjones deleted the nbytes-update branch July 22, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataArray.nbytes listed twice in API doc block nbytes does not return the true size for sparse variables
4 participants