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

Improve deprecation warning for DCDReader copying timestep #3923

Closed
hmacdope opened this issue Nov 15, 2022 · 2 comments · Fixed by #3888 or #3957
Closed

Improve deprecation warning for DCDReader copying timestep #3923

hmacdope opened this issue Nov 15, 2022 · 2 comments · Fixed by #3888 or #3957
Assignees
Labels
CZI-performance performance track of CZIEOSS4 grant deprecation Deprecated functionality to give advance warning for API changes. Format-DCD
Milestone

Comments

@hmacdope
Copy link
Member

Issue for proper paper trail

Add a deprecation warning for the DCDReader copying timestep unlike the other readers. Behaviour documented in #3889.

paraphrasing:

Expected behavior

The readers should all have the same behaviour with respect to iterating through the trajectory and changing the base values in the position attribute.

Actual behavior

The DCD reader creates a new copy of the current timestep which allows the coordinates in each frame to vary independently.

@hmacdope hmacdope added Format-DCD CZI-performance performance track of CZIEOSS4 grant labels Nov 15, 2022
@hmacdope hmacdope self-assigned this Nov 15, 2022
@hmacdope hmacdope mentioned this issue Nov 15, 2022
4 tasks
@hmacdope hmacdope moved this to In Progress in CZI Performance track Nov 15, 2022
@hmacdope hmacdope added this to the 2.4.0 milestone Nov 15, 2022
@orbeckst orbeckst added the deprecation Deprecated functionality to give advance warning for API changes. label Nov 22, 2022
@orbeckst orbeckst linked a pull request Nov 29, 2022 that will close this issue
4 tasks
orbeckst pushed a commit that referenced this issue Nov 29, 2022
* Fixes #3882 and #3923
* Improves the cython in the DCD reader to be more compiled.
* add annotate_cython setup.cfg option
* add types and fix is_periodic
* make int cast explicit
* change whence dict
* move DCD reader to no copy API and update tests accordingly
* add temporary buffer to DCD reader
* add module level directives
* pxdify libdcd: add new lib/formats/libdcd.pxd
* Add deprecation warning for `timestep` copying in DCDReader (Issue #3889)
* add DCD to public libmdanalysis API
* update tests
* update CHANGELOG
Repository owner moved this from In Progress to Done in CZI Performance track Nov 29, 2022
@orbeckst
Copy link
Member

orbeckst commented Dec 9, 2022

@IAlibay pointed out on discord that the current deprecation warning is not very helpful

DeprecationWarning: DCDReader currently makes independent timesteps by copying 
self.ts while other readers update self.ts inplace. This behaviour will be changed in 3.0 
to be the same as other readers 

I suggested to either link to a FAQ (to be written) or to this issue:

DeprecationWarning: DCDReader currently makes independent timesteps by copying 
self.ts while other readers update self.ts inplace. This behaviour will be changed in 3.0 
to be the same as other readers. Read more at 
https://github.com/MDAnalysis/mdanalysis/issues/3923 to learn if this change in 
behavior might affect you.

If we link to this issue from the DeprecationWarning then we need to be more clear about what changes for users, which code patterns are affected, and how to rewrite code.

@hmacdope hmacdope changed the title Add deprecation warning for DCDReader copying timestep ~~Add~~ Improve deprecation warning for DCDReader copying timestep Dec 12, 2022
@hmacdope hmacdope changed the title ~~Add~~ Improve deprecation warning for DCDReader copying timestep ~~ Add ~~ Improve deprecation warning for DCDReader copying timestep Dec 12, 2022
@hmacdope hmacdope changed the title ~~ Add ~~ Improve deprecation warning for DCDReader copying timestep Improve deprecation warning for DCDReader copying timestep Dec 12, 2022
@hmacdope
Copy link
Member Author

Re-opening for @orbeckst and @IAlibay comments.

We should:

Some helpful chatGPT suggestions include

This difference in behavior could lead to inconsistencies and confusion for users who are working with multiple reader classes, and it is being changed in the 3.0 release to make all readers behave the same way. In the 3.0 release, the DCDReader class will  update the self.ts attribute in place, just like the other readers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CZI-performance performance track of CZIEOSS4 grant deprecation Deprecated functionality to give advance warning for API changes. Format-DCD
Projects
2 participants