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.

Sign up for GitHub

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

Jump to bottom

Deprecate the ts=None optional kwarg in _read_next_timestep() for 3.0 #3928

Open
hmacdope opened this issue Nov 21, 2022 · 2 comments · May be fixed by #4334
Open

Deprecate the ts=None optional kwarg in _read_next_timestep() for 3.0 #3928

hmacdope opened this issue Nov 21, 2022 · 2 comments · May be fixed by #4334
Assignees
@hmacdope
Labels
Component-Readers CZI-performance performance track of CZIEOSS4 grant deprecation Deprecated functionality to give advance warning for API changes.
Milestone
Release 3.0

Comments

@hmacdope
Copy link
Member

Is your feature request related to a problem?

Several readers have a the following pattern

def _read_next_timestep(ts=None)

But IIRC the vast majority never use the ts=None kwarg for some kind of copy.

The existence of the ts=None kwarg is a bit of a slow and ugly and makes some optimisations difficult (see #3892 and copy confusion in DCDReader for in #3888 ).

Describe the solution you'd like

Deprecate for 3.0,
To quote @richardjgowers on #3892
"I think a Reader should hold a Timestep which is reused consistently, then people can manually copy if they want to." which makes a lot of sense to me.

Describe alternatives you've considered

Do nothing.
@hmacdope hmacdope added Component-Readers CZI-performance performance track of CZIEOSS4 grant labels Nov 21, 2022
@hmacdope hmacdope added this to the Release 3.0 milestone Nov 21, 2022
@hmacdope hmacdope self-assigned this Nov 21, 2022
@hmacdope hmacdope mentioned this issue Nov 21, 2022
Merged
4 tasks
@richardjgowers
Copy link
Member

The if ts=None pattern is everywhere in Readers, and I don't quite understand when that would be the case. This would be a nice historical artefact to fix in 3.0

@orbeckst
Copy link
Member

Sounds like built-in flexibility that we at least never used.

@orbeckst orbeckst added the deprecation Deprecated functionality to give advance warning for API changes. label Jan 3, 2023
@hmacdope hmacdope linked a pull request Nov 5, 2023 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hmacdope hmacdope

Labels
Component-Readers CZI-performance performance track of CZIEOSS4 grant deprecation Deprecated functionality to give advance warning for API changes.
Projects
CZI Performance track
Status: In Progress
Milestone
Release 3.0
Development

Successfully merging a pull request may close this issue.

Deprecate ts argument to _read_next_timestep hmacdope/mdanalysis
3 participants
@orbeckst @richardjgowers @hmacdope