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

Change return type of DataArray.chunks and Dataset.chunks to a dict #5846

Closed

Conversation

TomNicholas
Copy link
Member

Rectifies the the issue in #5843 by making DataArray.chunks and Variable.chunks consistent with Dataset.chunks. This would obviously need a deprecation cycle before it were merged.

Currently a WIP - I changed the behaviour but this obviously broke quite a few tests and I haven't looked at them yet.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2021

Unit Test Results

         6 files           6 suites   55m 45s ⏱️
16 226 tests 14 433 ✔️ 1 736 💤   57
90 552 runs  82 030 ✔️ 8 180 💤 342

For more details on these failures, see this check.

Results for commit 690bde8.

def chunks(self) -> Optional[Tuple[Tuple[int, ...], ...]]:
"""Block dimensions for this array's data or None if it's not a dask
array.
def chunks(self) -> Optional[Mapping[Hashable, Tuple[int, ...]]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def chunks(self) -> Optional[Mapping[Hashable, Tuple[int, ...]]]:
def chunks(self) -> Optional[Mapping[Any, Tuple[int, ...]]]:

We switched all these because of how mypy handles variance of key types

@max-sixty
Copy link
Collaborator

Code looks good.

I would generally weigh towards breaking changes if it improves consistency, but realize this is probably a close one. One synthesis of those is a new consistent method and we deprecate the old one. But not sure if there's a comparably good name than .chunks...

@TomNicholas TomNicholas mentioned this pull request Oct 26, 2021
5 tasks
@TomNicholas
Copy link
Member Author

Superceded by #5900

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.

Why are da.chunks and ds.chunks properties inconsistent?
2 participants