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

Deprecate 'with dset.astype()' #1842

Merged
merged 5 commits into from
Nov 2, 2021

Conversation

takluyver
Copy link
Member

@takluyver takluyver commented Mar 3, 2021

h5py 3.0 introduced the ability to slice an astype wrapper directly (ds.astype(np.int32)[:]). I'd like to remove the old pattern of using astype() as a context manager with mutable state, and get rid of the thread-local storage it requires.

Edit: I am happy if we want to give people more time to update to 3.x before adding this warning. We released 3.0 in October 2020. The 3.3 release may be around 6 months after that, depending on how eager we are to do it.

h5py 3.0 introduced the ability to slice an astype wrapper directly
(ds.astype(np.int32)[:]). I'd like to remove the old pattern of using
astype() as a context manager with mutable state, and get rid of the
thread-local storage it requires.
@takluyver takluyver added this to the 3.3 milestone Mar 3, 2021
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #1842 (c1bdb52) into master (1340914) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1842      +/-   ##
==========================================
+ Coverage   88.38%   88.39%   +0.01%     
==========================================
  Files          17       17              
  Lines        2281     2284       +3     
==========================================
+ Hits         2016     2019       +3     
  Misses        265      265              
Impacted Files Coverage Δ
h5py/_hl/dataset.py 92.56% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1340914...c1bdb52. Read the comment docs.

@takluyver
Copy link
Member Author

Add the warning for 3.3, or wait a while longer?

This is trickier than #1839. The sooner we start pushing people away from this, the sooner we can get rid of the thread-locals piece. But the only way to keep code compatible with h5py 2.x and 3.x is to use the older style (with dset.astype(np.int32):). This also means it's necessary for any code that still has to support Python 2.

I'm leaning towards leaving this a bit longer - maybe add the warning a year after h5py 3.0 was released.

tacaswell
tacaswell previously approved these changes Jun 18, 2021
@tacaswell tacaswell modified the milestones: 3.3, 3.4 Jun 18, 2021
@tacaswell tacaswell dismissed their stale review June 18, 2021 21:46

Changed my mind about timeline.

@tacaswell
Copy link
Member

I'm sold that we should let people have a 12 month support window for older versions of h5py. This should wait until Nov 2021 (which I presume will be h5py3.4).

@takluyver takluyver modified the milestones: 3.4, 3.5 Aug 20, 2021
@takluyver takluyver modified the milestones: 3.5, 3.6 Oct 20, 2021
@aragilar
Copy link
Member

aragilar commented Nov 2, 2021

Should we merge this now?

@takluyver
Copy link
Member Author

Yes, it has been a year since 3.0 was released with the new arr = dset.astype(np.int32)[:10] pattern, which was the delay we agreed on before warning. We can allow it with a warning for another 6 or 12 months, I guess.

@takluyver takluyver merged commit c62b34c into h5py:master Nov 2, 2021
@takluyver takluyver deleted the deprecate-with-astype branch November 2, 2021 12:01
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.

None yet

3 participants