Remove ts from writers by IAlibay · Pull Request #2754 · MDAnalysis/mdanalysis

IAlibay · 2020-06-14T09:30:08Z

Towards #2739

Changes made in this Pull Request:

Removed deprecated use of timestep from writers.
Removed Writer.write_next_timestep
Removes support for passing atoms to XYZWriter (code paths here no longer worked since we removed support for Timestep).
Partial py2 removals for coordinates (rest to follow in a separate PR).
Mostly aligned error checking for using Timestep on writers.

To do:

Fix broken tests (chainreader, netcdf, lammps, trz, xdr, xyz currently failing).
Add checks for raising TypeError on calling with timestep?

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

IAlibay · 2020-06-14T15:13:01Z

package/MDAnalysis/coordinates/null.py

        pass

    def _write_next_frame(self, obj):
+        try:


I went ahead and added this check to the null writer. My understanding is that the null writer should behave like all other writers, so it should complain if we pass it a Timestep. Thoughts anyone?

richardjgowers · 2020-06-14T15:20:43Z

Sounds sane

…

On Sun, Jun 14, 2020 at 16:13, Irfan Alibay ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In package/MDAnalysis/coordinates/null.py <#2754 (comment)> : > @@ -57,4 +57,9 @@ def __init__(self, filename, **kwargs): pass def _write_next_frame(self, obj): + try: I went ahead and added this check to the null writer. My understanding is that the null writer should behave like all other writers, so it should complain if we pass it a Timestep. Thoughts anyone? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2754 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB7H37PP2YZLKSKZQLTRWTSIVANCNFSM4N5LZTIQ> .

codecov · 2020-06-14T16:45:28Z

Codecov Report

Merging #2754 into develop will increase coverage by 0.01%.
The diff coverage is 93.27%.

@@             Coverage Diff             @@
##           develop    #2754      +/-   ##
===========================================
+ Coverage    91.17%   91.19%   +0.01%     
===========================================
  Files          176      176              
  Lines        23798    23767      -31     
  Branches      3134     3118      -16     
===========================================
- Hits         21699    21674      -25     
+ Misses        1476     1473       -3     
+ Partials       623      620       -3

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/CRD.py	`79.20% <71.42%> (-0.60%)`	⬇️
package/MDAnalysis/coordinates/TRZ.py	`83.58% <78.57%> (+1.46%)`	⬆️
package/MDAnalysis/coordinates/base.py	`93.67% <85.71%> (+0.10%)`	⬆️
package/MDAnalysis/coordinates/GRO.py	`93.95% <92.30%> (-1.50%)`	⬇️
package/MDAnalysis/coordinates/DCD.py	`97.20% <100.00%> (-0.06%)`	⬇️
package/MDAnalysis/coordinates/FHIAIMS.py	`93.75% <100.00%> (-1.94%)`	⬇️
package/MDAnalysis/coordinates/MOL2.py	`94.48% <100.00%> (+0.17%)`	⬆️
package/MDAnalysis/coordinates/NAMDBIN.py	`100.00% <100.00%> (ø)`
package/MDAnalysis/coordinates/PDBQT.py	`84.53% <100.00%> (+0.66%)`	⬆️
package/MDAnalysis/coordinates/PQR.py	`91.95% <100.00%> (+0.18%)`	⬆️
... and 12 more

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 085ac0f...b1f5dc9. Read the comment docs.

richardjgowers

A few comments but looking good

richardjgowers · 2020-06-14T19:40:43Z

package/MDAnalysis/coordinates/GRO.py

           *resName* and *atomName* are truncated to a maximum of 5 characters
        .. versionchanged:: 0.16.0
           `frame` kwarg has been removed
-        .. deprecated:: 1.0.0


I think we’re meant to keep the old tag and just keep appending new stuff

This came up before in: #2494 (comment) and I think the idea at the time was to remove deprecation notices since they get replaced by the versionchanged.

I am glad that you have a good memory @IAlibay !

I added a section under Writing Documentation: Documenting changes to capture what I believe is our current consensus. It's up for discussion, of course.

@lilyminium I know that the User Guide copied most of the wiki style guide so I created PR MDAnalysis/UserGuide#76 with these changes.

richardjgowers · 2020-06-14T19:41:40Z

package/MDAnalysis/coordinates/DCD.py

-            ts = ag
-        else:
+        try:
+            ts = ag.ts


Constructing an AtomGroup timestep ain’t free, so maybe use ag.positions to grab the positions etc

So from a quick look, I think DCD, TRJ, TRR, TRZ, XTC, XYZ, and chemfiles all do something similar. In the interest of not making this PR too hard to follow, would you be ok if I opened an issue for this and dealt with it in a separate PR?

richardjgowers · 2020-06-14T19:43:04Z

package/MDAnalysis/coordinates/MOL2.py

        # Issue 2717
-        obj = obj.atoms
+        try:
+            obj = obj.atoms


I’d hope stuff is type checked by this point

Unfortunately this is the failure point if you pass a Timestep object to the MOL2Writer. I could move it up to _write_next_frame if you think it's more appropriate?

richardjgowers · 2020-06-14T19:44:07Z

package/MDAnalysis/coordinates/NAMDBIN.py

+           Use AtomGroup or Universe as an input instead.
        """
-        # TODO 2.0: Remove Timestep logic
-        if isinstance(obj, base.Timestep):


Same here, put a type check in base.Write

Taken from https://github.com/MDAnalysis/mdanalysis/wiki/WritingDocumentation#documenting-changes in response to MDAnalysis/mdanalysis#2754 (comment)

* Removes deprecated timestep input to writers

IAlibay added 5 commits June 13, 2020 23:33

Removes deprecated timestep input to writers

8399aa7

change base ts write tests

81ec9cb

Remove except..as.. and fixes some tests

f805970

Fix chainreader ts tests

be270a0

fix xdr ts write tests

c56657e

IAlibay mentioned this pull request Jun 14, 2020

Deprecated and slated for 2.0 removal #2739

Closed

5 tasks

IAlibay added 6 commits June 14, 2020 13:33

fixes netcdf tests

7ddb85c

Fix xyz

35d9357

Uniform error on writing ts

ab831ca

Fixes null writer tests for ts

2652daa

Adds changelog entry for Writer.write(ts) removal

7eb12f1

Fix XYZWriter docstring

ce1b9fc

IAlibay commented Jun 14, 2020

View reviewed changes

Aligns error for passing timestep to mol2writer

bcc960b

IAlibay marked this pull request as ready for review June 14, 2020 15:23

Merge branch 'develop' into remove-ts-writers

b1f5dc9

richardjgowers requested changes Jun 14, 2020

View reviewed changes

orbeckst added the remove-2.0 deprecated in 1.0 and to be removed in 2.0 label Jun 15, 2020

richardjgowers approved these changes Jun 15, 2020

View reviewed changes

richardjgowers merged commit 734d32a into MDAnalysis:develop Jun 15, 2020

IAlibay mentioned this pull request Jun 15, 2020

Make writers use atomgroups internally #2757

Open

orbeckst mentioned this pull request Jun 15, 2020

updated doc guide with "documenting changes" MDAnalysis/UserGuide#76

Merged

IAlibay deleted the remove-ts-writers branch June 15, 2020 10:37

This was referenced Nov 17, 2020

Improve the performance of ParmEd converter. (Fix #3028) #3029

Merged

Remove support for ts as an input to the ParmedConverter #3031

Closed

PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021

Remove ts from writers (MDAnalysis#2754)

33db5d6

* Removes deprecated timestep input to writers

fiona-naughton added the maintainability label Sep 25, 2023

fiona-naughton added Component-Writers deprecation Deprecated functionality to give advance warning for API changes. labels Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ts from writers#2754

Remove ts from writers#2754
richardjgowers merged 13 commits intoMDAnalysis:developfrom
IAlibay:remove-ts-writers

IAlibay commented Jun 14, 2020 •

edited

Loading

Uh oh!

IAlibay Jun 14, 2020

Uh oh!

richardjgowers commented Jun 14, 2020 via email

Uh oh!

codecov bot commented Jun 14, 2020 •

edited

Loading

Uh oh!

richardjgowers left a comment

Uh oh!

richardjgowers Jun 14, 2020

Uh oh!

IAlibay Jun 14, 2020

Uh oh!

orbeckst Jun 15, 2020

Uh oh!

richardjgowers Jun 14, 2020

Uh oh!

IAlibay Jun 14, 2020

Uh oh!

richardjgowers Jun 14, 2020

Uh oh!

IAlibay Jun 14, 2020

Uh oh!

richardjgowers Jun 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

IAlibay commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardjgowers commented Jun 14, 2020 via email

Uh oh!

codecov bot commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richardjgowers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IAlibay commented Jun 14, 2020 •

edited

Loading

codecov bot commented Jun 14, 2020 •

edited

Loading