From d2f330c5da26600a6a1c495f1a64b8eac8e3c3bc Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Tue, 19 May 2020 16:19:39 +0200
Subject: [PATCH 01/17] add failing test

---
 package/MDAnalysis/coordinates/PDB.py             |  1 +
 testsuite/MDAnalysisTests/coordinates/test_pdb.py | 15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 3463d223e58..5990acdf406 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -651,6 +651,7 @@ def _write_pdb_header(self):
             self.REMARK(*remarks)
         except AttributeError:
             pass
+
         self.CRYST1(self.convert_dimensions_to_unitcell(u.trajectory.ts))
 
     def _check_pdb_coordinates(self):
diff --git a/testsuite/MDAnalysisTests/coordinates/test_pdb.py b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
index e1eef78d864..48dc3b15dee 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_pdb.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
@@ -47,6 +47,7 @@
 
 class TestPDBReader(_SingleFrameReader):
     __test__ = True
+
     def setUp(self):
         # can lead to race conditions when testing in parallel
         self.universe = mda.Universe(RefAdKSmall.filename)
@@ -190,6 +191,10 @@ def universe2(self):
     def universe3(self):
         return mda.Universe(PDB)
 
+    @pytest.fixture
+    def universe4(self):
+        return mda.fetch_mmtf("2BBM")
+
     @pytest.fixture
     def outfile(self, tmpdir):
         return str(tmpdir.mkdir("PDBWriter").join('primitive-pdb-writer' + self.ext))
@@ -291,6 +296,14 @@ def test_write_single_frame_AtomGroup(self, universe2, outfile):
                                                "agree with original coordinates from frame %d" %
                                                u.trajectory.frame)
 
+    def test_write_nodimes(self, universe4, outfile):
+        u = universe4
+
+        u.atoms.write(outfile)
+
+        uout = mda.Universe(outfile)
+
+
     def test_check_coordinate_limits_min(self, universe, outfile):
         """Test that illegal PDB coordinates (x <= -999.9995 A) are caught
         with ValueError (Issue 57)"""
@@ -513,6 +526,7 @@ def helper(atoms, bonds):
                                               "the test reference; len(actual) is %d, len(desired) "
                                               "is %d" % (len(u._topology.bonds.values), len(desired)))
 
+
 def test_conect_bonds_all(tmpdir):
     conect = mda.Universe(CONECT, guess_bonds=True)
 
@@ -528,6 +542,7 @@ def test_conect_bonds_all(tmpdir):
 
     # assert_equal(len([b for b in conect.bonds if not b.is_guessed]), 1922)
 
+
 def test_write_bonds_partial(tmpdir):
     u = mda.Universe(CONECT)
     # grab all atoms with bonds

From 5bf934fe989ac8cad775aaf67fcc0467c30b3d1a Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Tue, 19 May 2020 16:27:09 +0200
Subject: [PATCH 02/17] fix

---
 package/MDAnalysis/coordinates/PDB.py             |  8 +++++++-
 testsuite/MDAnalysisTests/coordinates/test_pdb.py | 12 ++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 5990acdf406..6bc2d313ccd 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -631,6 +631,11 @@ def _write_pdb_title(self):
                        "".format(self.start, self.remarks))
 
     def _write_pdb_header(self):
+        """
+        .. versionchanged: 1.0.0
+           Write CRYST1 only if :code:`u.trajectory.ts.dimensions`
+           is not :code:`None`.
+        """
         if self.first_frame_done == True:
             return
 
@@ -652,7 +657,8 @@ def _write_pdb_header(self):
         except AttributeError:
             pass
 
-        self.CRYST1(self.convert_dimensions_to_unitcell(u.trajectory.ts))
+        if u.trajectory.ts.dimensions is not None:
+            self.CRYST1(self.convert_dimensions_to_unitcell(u.trajectory.ts))
 
     def _check_pdb_coordinates(self):
         """Check if the coordinate values fall within the range allowed for PDB files.
diff --git a/testsuite/MDAnalysisTests/coordinates/test_pdb.py b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
index 48dc3b15dee..bae148979e3 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_pdb.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
@@ -303,6 +303,18 @@ def test_write_nodimes(self, universe4, outfile):
 
         uout = mda.Universe(outfile)
 
+        assert_equal(
+            uout.trajectory.n_frames, 1,
+            err_msg="Output PDB should only contain a single frame"
+        )
+
+        assert_almost_equal(
+            u.atoms.positions, uout.atoms.positions,
+            self.prec,
+            err_msg="Written coordinates do not "
+                "agree with original coordinates from frame %d" %
+                u.trajectory.frame
+        )
 
     def test_check_coordinate_limits_min(self, universe, outfile):
         """Test that illegal PDB coordinates (x <= -999.9995 A) are caught

From 1e1280b66fe030475a812e8eef760ef57e4a5cee Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Tue, 19 May 2020 16:34:14 +0200
Subject: [PATCH 03/17] simpler dimensions check

---
 package/MDAnalysis/coordinates/PDB.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 6bc2d313ccd..096740526d4 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -657,7 +657,7 @@ def _write_pdb_header(self):
         except AttributeError:
             pass
 
-        if u.trajectory.ts.dimensions is not None:
+        if u.dimensions is not None:
             self.CRYST1(self.convert_dimensions_to_unitcell(u.trajectory.ts))
 
     def _check_pdb_coordinates(self):

From d670fbbf43da897cae174face3e4199ba23bd34c Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Tue, 19 May 2020 16:37:58 +0200
Subject: [PATCH 04/17] changelog

---
 package/CHANGELOG | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/package/CHANGELOG b/package/CHANGELOG
index b46587bfe73..2463be63e3d 100644
--- a/package/CHANGELOG
+++ b/package/CHANGELOG
@@ -22,6 +22,8 @@ mm/dd/yy richardjgowers, kain88-de, lilyminium, p-j-smith, bdice, joaomcteixeira
   * 0.21.0
 
 Fixes
+  * `PDBWriter` skips `CRYST1` record when `u.dimensions` is 
+    `None` (Issue #2679, PR #2685)
   * Fixed retrieval of auxiliary information after getting the last timestep
     of the trajectory (Issue #2674, PR #2683).
   * n_components correctly selects PCA components (Issue #2623)

From ecb8612aceb51d411d8d996101676e58e4f1d785 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Tue, 19 May 2020 21:52:27 +0200
Subject: [PATCH 05/17] address pr comments

---
 package/MDAnalysis/coordinates/PDB.py             | 14 +++++++++++---
 testsuite/MDAnalysisTests/coordinates/test_pdb.py | 11 +++++++++--
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 096740526d4..6b3e2790db1 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -632,10 +632,15 @@ def _write_pdb_title(self):
 
     def _write_pdb_header(self):
         """
+        Write PDB header.
+
+        CRYST1 field is skipped if if :code:`u.dimensions` is  :code:`None`.
+
         .. versionchanged: 1.0.0
-           Write CRYST1 only if :code:`u.trajectory.ts.dimensions`
-           is not :code:`None`.
+           Write CRYST1 only if :code:`u.dimensions`
+           is not :code:`None` (Issue #2679).
         """
+
         if self.first_frame_done == True:
             return
 
@@ -667,7 +672,10 @@ def _check_pdb_coordinates(self):
         already been written (in multi-frame mode) adds a REMARK instead of the
         coordinates and closes the file.
 
-        Raises :exc:`ValueError` if the coordinates fail the check.
+        Raises
+        ------
+        ValueError
+            if the coordinates fail the check.
 
         .. versionchanged: 1.0.0
             Check if :attr:`filename` is `StringIO` when attempting to remove
diff --git a/testsuite/MDAnalysisTests/coordinates/test_pdb.py b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
index bae148979e3..4d2d479a09a 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_pdb.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
@@ -296,9 +296,16 @@ def test_write_single_frame_AtomGroup(self, universe2, outfile):
                                                "agree with original coordinates from frame %d" %
                                                u.trajectory.frame)
 
-    def test_write_nodimes(self, universe4, outfile):
-        u = universe4
+    def test_write_nodims(self, universe4, outfile):
+        """
+        Test :code:`PDBWriter` for universe without cell dimensions.
 
+        Notes
+        -----
+        Test fix for Issue #2679.
+        """
+
+        u = universe4
         u.atoms.write(outfile)
 
         uout = mda.Universe(outfile)

From 62fe9d3e316a0e252ebb149685a580ffaed51486 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Tue, 19 May 2020 22:09:44 +0200
Subject: [PATCH 06/17] improve docstrings

---
 package/MDAnalysis/coordinates/PDB.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 6b3e2790db1..6dac9f0302b 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -634,7 +634,13 @@ def _write_pdb_header(self):
         """
         Write PDB header.
 
-        CRYST1 field is skipped if if :code:`u.dimensions` is  :code:`None`.
+        The HEADER record is set to :code: `trajectory.header`.
+        The TITLE record explicitly mentions MDAnalysis and contains
+        information about trajectory frame(s).
+        The COMPND record is set to :code:`trajectory.compound`.
+        The REMARKS records are set to :code:`u.trajectory.remarks`
+        The CRYST1 record specifies the unit cell. This record is skipped 
+        if :code:`u.dimensions` is  :code:`None`.
 
         .. versionchanged: 1.0.0
            Write CRYST1 only if :code:`u.dimensions`

From e1dba87e1360b1e26fdeeefc641bcfe2a328b0de Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 22 May 2020 16:15:23 +0200
Subject: [PATCH 07/17] fix remark and test

---
 package/MDAnalysis/coordinates/XYZ.py         | 33 ++++++++++-----
 .../MDAnalysisTests/coordinates/test_xyz.py   | 40 +++++++++++++++++++
 2 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index abd72e14fce..2b4eb48e474 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -54,7 +54,7 @@
 XYZ File format
 ---------------
 
-Definiton used by the :class:`XYZReader` and :class:`XYZWriter` (and
+Definition used by the :class:`XYZReader` and :class:`XYZWriter` (and
 the `VMD xyzplugin`_ from whence the definition was taken)::
 
     [ comment line            ] !! NOT IMPLEMENTED !! DO NOT INCLUDE
@@ -139,28 +139,29 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=True,
             used when a trajectory is written from raw
             :class:`Timestep` objects which do not contain atom
             information. If you write a :class:`AtomGroup` with
-            :meth:`XYZWriter.write` then atom information is taken
+            :math:`XYZWriter.write` then atom information is taken
             at each step and *atoms* is ignored.
         convert_units : bool (optional)
             convert quantities to default MDAnalysis units of Angstrom upon
             writing  [``True``]
         remark: str (optional)
             single line of text ("molecule name"). By default writes MDAnalysis
-            version
+            version and frame
         """
         self.filename = filename
+        self.remark = remark
         self.n_atoms = n_atoms
         self.convert_units = convert_units
 
         self.atomnames = self._get_atoms_elements_or_names(atoms)
-        default_remark = "Written by {0} (release {1})".format(
-            self.__class__.__name__, __version__)
-        self.remark = default_remark if remark is None else remark
+
         # can also be gz, bz2
         self._xyz = util.anyopen(self.filename, 'wt')
 
     def _get_atoms_elements_or_names(self, atoms):
-        """Return a list of atom elements (if present) or fallback to atom names"""
+        """
+        Return a list of atom elements (if present) or fallback to atom names
+        """
         # Default case
         if atoms is None:
             return itertools.cycle(('X',))
@@ -190,8 +191,8 @@ def close(self):
     def write(self, obj):
         """Write object `obj` at current trajectory frame to file.
 
-        Atom elements (or names) in the output are taken from the `obj` or default
-        to the value of the `atoms` keyword supplied to the
+        Atom elements (or names) in the output are taken from the `obj` or
+        default to the value of the `atoms` keyword supplied to the
         :class:`XYZWriter` constructor.
 
         Parameters
@@ -259,8 +260,20 @@ def write_next_timestep(self, ts=None):
         else:
             coordinates = ts.positions
 
+        # Write number of atoms
         self._xyz.write("{0:d}\n".format(ts.n_atoms))
-        self._xyz.write("frame {0}\n".format(ts.frame))
+
+        # Write remark
+        if self.remark is None:
+            remark = "Written by {0} (release {1})".format(
+                self.__class__.__name__, __version__
+                ) + "| Frame {0}\n".format(ts.frame)
+
+            self._xyz.write(remark)
+        else:
+            self._xyz.write(self.remark.strip() + "\n")
+
+        # Write content
         for atom, (x, y, z) in zip(self.atomnames, coordinates):
             self._xyz.write("{0!s:>8}  {1:10.5f} {2:10.5f} {3:10.5f}\n"
                             "".format(atom, x, y, z))
diff --git a/testsuite/MDAnalysisTests/coordinates/test_xyz.py b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
index 7f39509be0c..bd79e5387a3 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_xyz.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
@@ -106,6 +106,46 @@ def test_no_conversion(self, ref, reader, tmpdir):
                     w.write(ts)
             self._check_copy(outfile, ref, reader)
 
+    def test_remark(self, ref, tmpdir):
+        """
+        Test remark handling with custom remark.
+        """
+
+        u = mda.Universe(ref.topology, ref.trajectory)
+
+        outfile = "write-remark.xyz"
+        remark = "This is a custom remark."
+
+        with tmpdir.as_cwd():
+            u.atoms.write(outfile, remark=remark)
+
+            with open(outfile, "r") as xyzout:
+                lines = xyzout.readlines()
+
+                print(lines)
+
+                assert lines[1].strip() == remark
+
+    def test_emptyremark(self, ref, tmpdir):
+        """
+        Test remark handling with empty remark.
+        """
+
+        u = mda.Universe(ref.topology, ref.trajectory)
+
+        outfile = "write-remark-empty.xyz"
+        remark = ""
+
+        with tmpdir.as_cwd():
+            u.atoms.write(outfile, remark=remark)
+
+            with open(outfile, "r") as xyzout:
+                lines = xyzout.readlines()
+
+                print(lines)
+
+                assert lines[1].strip() == remark
+
 
 class XYZ_BZ_Reference(XYZReference):
     def __init__(self):

From 60e0583aac4a36d1b507be8cb4df206cdd1621b1 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 22 May 2020 16:22:45 +0200
Subject: [PATCH 08/17] better tests

---
 package/MDAnalysis/coordinates/XYZ.py         |  4 +-
 .../MDAnalysisTests/coordinates/test_xyz.py   | 39 ++++++-------------
 2 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index 2b4eb48e474..d559512bed4 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -265,9 +265,9 @@ def write_next_timestep(self, ts=None):
 
         # Write remark
         if self.remark is None:
-            remark = "Written by {0} (release {1})".format(
+            remark = "Written by MDAnalysis {0} (release {1})".format(
                 self.__class__.__name__, __version__
-                ) + "| Frame {0}\n".format(ts.frame)
+                ) + " | Frame {0}\n".format(ts.frame)
 
             self._xyz.write(remark)
         else:
diff --git a/testsuite/MDAnalysisTests/coordinates/test_xyz.py b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
index bd79e5387a3..1a8246f715c 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_xyz.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
@@ -38,6 +38,7 @@
                                               BaseWriterTest)
 from MDAnalysisTests import make_Universe
 
+from MDAnalysis import __version__
 
 class XYZReference(BaseReference):
     def __init__(self):
@@ -106,45 +107,29 @@ def test_no_conversion(self, ref, reader, tmpdir):
                     w.write(ts)
             self._check_copy(outfile, ref, reader)
 
-    def test_remark(self, ref, tmpdir):
+    @pytest.mark.parametrize("remarkout, remarkin", 
+        [   
+            2 * ["Curstom Remark"],
+            2 * [""],
+            [None, "Written by MDAnalysis XYZWriter (release {0}) | Frame 0".format(__version__)],
+        ]
+    )
+    def test_remark(self, remarkout, remarkin, ref, tmpdir):
         """
-        Test remark handling with custom remark.
+        Test remark handling.
         """
 
         u = mda.Universe(ref.topology, ref.trajectory)
 
         outfile = "write-remark.xyz"
-        remark = "This is a custom remark."
 
         with tmpdir.as_cwd():
-            u.atoms.write(outfile, remark=remark)
+            u.atoms.write(outfile, remark=remarkout)
 
             with open(outfile, "r") as xyzout:
                 lines = xyzout.readlines()
 
-                print(lines)
-
-                assert lines[1].strip() == remark
-
-    def test_emptyremark(self, ref, tmpdir):
-        """
-        Test remark handling with empty remark.
-        """
-
-        u = mda.Universe(ref.topology, ref.trajectory)
-
-        outfile = "write-remark-empty.xyz"
-        remark = ""
-
-        with tmpdir.as_cwd():
-            u.atoms.write(outfile, remark=remark)
-
-            with open(outfile, "r") as xyzout:
-                lines = xyzout.readlines()
-
-                print(lines)
-
-                assert lines[1].strip() == remark
+                assert lines[1].strip() == remarkin
 
 
 class XYZ_BZ_Reference(XYZReference):

From 0c55bf2673b013744c3911acad1a156cedf80e38 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 22 May 2020 16:25:48 +0200
Subject: [PATCH 09/17] versionchanged

---
 package/MDAnalysis/coordinates/XYZ.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index d559512bed4..2534fce16c7 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -147,6 +147,9 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=True,
         remark: str (optional)
             single line of text ("molecule name"). By default writes MDAnalysis
             version and frame
+
+        .. versionchanged:: 1.0.0
+           Removed unused :code:`default_remark` variable.
         """
         self.filename = filename
         self.remark = remark
@@ -230,7 +233,13 @@ def write(self, obj):
         self.write_next_timestep(ts)
 
     def write_next_timestep(self, ts=None):
-        """Write coordinate information in *ts* to the trajectory"""
+        """
+        Write coordinate information in *ts* to the trajectory
+
+
+        .. versionchanged:: 1.0.0
+           Print out :code:`remark` if present, otherwise use generic one.
+        """
         if ts is None:
             if not hasattr(self, 'ts'):
                 raise NoDataError('XYZWriter: no coordinate data to write to '

From b968a51a9f2c31a2052a4f2b927e5064a264c94b Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 22 May 2020 16:27:43 +0200
Subject: [PATCH 10/17] changelog

---
 package/CHANGELOG                     | 1 +
 package/MDAnalysis/coordinates/XYZ.py | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/package/CHANGELOG b/package/CHANGELOG
index 2463be63e3d..9f37f4226b4 100644
--- a/package/CHANGELOG
+++ b/package/CHANGELOG
@@ -22,6 +22,7 @@ mm/dd/yy richardjgowers, kain88-de, lilyminium, p-j-smith, bdice, joaomcteixeira
   * 0.21.0
 
 Fixes
+  * Use user-provided `remark` in `XYZWriter` (Issue #2692)
   * `PDBWriter` skips `CRYST1` record when `u.dimensions` is 
     `None` (Issue #2679, PR #2685)
   * Fixed retrieval of auxiliary information after getting the last timestep
diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index 2534fce16c7..6fe2ecd5551 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -149,7 +149,7 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=True,
             version and frame
 
         .. versionchanged:: 1.0.0
-           Removed unused :code:`default_remark` variable.
+           Removed unused :code:`default_remark` variable (Issue #2692).
         """
         self.filename = filename
         self.remark = remark
@@ -238,7 +238,8 @@ def write_next_timestep(self, ts=None):
 
 
         .. versionchanged:: 1.0.0
-           Print out :code:`remark` if present, otherwise use generic one.
+           Print out :code:`remark` if present, otherwise use generic one 
+           (Issue #2692).
         """
         if ts is None:
             if not hasattr(self, 'ts'):

From c0be2d0d1c865ecfa29fe0bfbcaaa6ba09017a86 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 5 Jun 2020 13:06:44 +0200
Subject: [PATCH 11/17] revert PDB change

---
 package/MDAnalysis/coordinates/PDB.py | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 277c99a61d3..2e2a46baa15 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -702,6 +702,29 @@ def _write_pdb_header(self):
         except AttributeError:
             pass
 
+                # FIXME: Values for meaningless cell dimensions are not consistent.
+        # FIXME: See Issue #2698. Here we check for both None and zeros
+        if u.dimensions is None or np.allclose(u.dimensions, np.zeros(6)):
+            # Unitary unit cell by default. See PDB standard:
+            # http://www.wwpdb.org/documentation/file-format-content/format33/sect8.html#CRYST1
+            self.CRYST1(np.array([1.0, 1.0, 1.0, 90.0, 90.0, 90.0]))
+
+            # Add CRYST1 REMARK (285)
+            # The SCALE record is not included
+            # (We are only implementing a subset of the PDB standard)
+            self.REMARK("285 UNITARY VALUES FOR THE UNIT CELL AUTOMATICALLY SET")
+            self.REMARK("285 BY MDANALYSIS PDBWRITER BECAUSE UNIT CELL INFORMATION")
+            self.REMARK("285 WAS MISSING.")
+            self.REMARK("285 PROTEIN DATA BANK CONVENTIONS REQUIRE THAT")
+            self.REMARK("285 CRYST1 RECORD IS INCLUDED, BUT THE VALUES ON")
+            self.REMARK("285 THIS RECORD ARE MEANINGLESS.")
+
+            warnings.warn("Unit cell dimensions not found. "
+                          "CRYST1 record set to unitary values.")
+
+        else:
+            self.CRYST1(self.convert_dimensions_to_unitcell(u.trajectory.ts))
+
     def _check_pdb_coordinates(self):
         """Check if the coordinate values fall within the range allowed for PDB files.
 

From 05784c00146f289150e22c026c23d35b3a172354 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 5 Jun 2020 13:07:37 +0200
Subject: [PATCH 12/17] make diff zero

---
 package/MDAnalysis/coordinates/PDB.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/PDB.py b/package/MDAnalysis/coordinates/PDB.py
index 2e2a46baa15..23f8914c79b 100644
--- a/package/MDAnalysis/coordinates/PDB.py
+++ b/package/MDAnalysis/coordinates/PDB.py
@@ -702,7 +702,7 @@ def _write_pdb_header(self):
         except AttributeError:
             pass
 
-                # FIXME: Values for meaningless cell dimensions are not consistent.
+        # FIXME: Values for meaningless cell dimensions are not consistent.
         # FIXME: See Issue #2698. Here we check for both None and zeros
         if u.dimensions is None or np.allclose(u.dimensions, np.zeros(6)):
             # Unitary unit cell by default. See PDB standard:

From 433881e338f5183e4acf8922779aeec1589a0e79 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 5 Jun 2020 13:11:09 +0200
Subject: [PATCH 13/17] remove spurious PDB universe

---
 package/MDAnalysis/coordinates/XYZ.py             | 4 +---
 testsuite/MDAnalysisTests/coordinates/test_pdb.py | 4 ----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index 6fe2ecd5551..79fef5f9b38 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -162,9 +162,7 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=True,
         self._xyz = util.anyopen(self.filename, 'wt')
 
     def _get_atoms_elements_or_names(self, atoms):
-        """
-        Return a list of atom elements (if present) or fallback to atom names
-        """
+        """Return a list of atom elements (if present) or fallback to atom names"""
         # Default case
         if atoms is None:
             return itertools.cycle(('X',))
diff --git a/testsuite/MDAnalysisTests/coordinates/test_pdb.py b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
index 8120176ec90..0c20fb59e23 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_pdb.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_pdb.py
@@ -208,10 +208,6 @@ def universe_and_expected_dims(self, request):
 
         return mda.Universe(filein), expected_dims
 
-    @pytest.fixture
-    def universe4(self):
-        return mda.fetch_mmtf("2BBM")
-
     @pytest.fixture
     def outfile(self, tmpdir):
         return str(tmpdir.mkdir("PDBWriter").join('primitive-pdb-writer' + self.ext))

From c3444772356cfa4e7c0dd12b91e52b056357b53e Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 5 Jun 2020 13:11:54 +0200
Subject: [PATCH 14/17] fix doc

---
 package/MDAnalysis/coordinates/XYZ.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index 79fef5f9b38..5a9427e8a59 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -139,7 +139,7 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=True,
             used when a trajectory is written from raw
             :class:`Timestep` objects which do not contain atom
             information. If you write a :class:`AtomGroup` with
-            :math:`XYZWriter.write` then atom information is taken
+            :meth:`XYZWriter.write` then atom information is taken
             at each step and *atoms* is ignored.
         convert_units : bool (optional)
             convert quantities to default MDAnalysis units of Angstrom upon

From 0abe440fac947ea0ab1e4e9ed6187cbdded14e94 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Fri, 5 Jun 2020 13:13:43 +0200
Subject: [PATCH 15/17] final doc fixes

---
 package/MDAnalysis/coordinates/XYZ.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index 5a9427e8a59..16f235bf9d0 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -149,7 +149,7 @@ def __init__(self, filename, n_atoms=None, atoms=None, convert_units=True,
             version and frame
 
         .. versionchanged:: 1.0.0
-           Removed unused :code:`default_remark` variable (Issue #2692).
+           Removed :code:`default_remark` variable (Issue #2692).
         """
         self.filename = filename
         self.remark = remark
@@ -234,7 +234,6 @@ def write_next_timestep(self, ts=None):
         """
         Write coordinate information in *ts* to the trajectory
 
-
         .. versionchanged:: 1.0.0
            Print out :code:`remark` if present, otherwise use generic one 
            (Issue #2692).

From f25a8946f2f78877ad62dc9d4e4f12f44ccc1375 Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Sat, 6 Jun 2020 14:25:38 +0200
Subject: [PATCH 16/17] address pr comments

---
 package/MDAnalysis/coordinates/XYZ.py             | 5 ++---
 testsuite/MDAnalysisTests/coordinates/test_xyz.py | 6 +-----
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/package/MDAnalysis/coordinates/XYZ.py b/package/MDAnalysis/coordinates/XYZ.py
index 16f235bf9d0..4eb6af44502 100644
--- a/package/MDAnalysis/coordinates/XYZ.py
+++ b/package/MDAnalysis/coordinates/XYZ.py
@@ -272,9 +272,8 @@ def write_next_timestep(self, ts=None):
 
         # Write remark
         if self.remark is None:
-            remark = "Written by MDAnalysis {0} (release {1})".format(
-                self.__class__.__name__, __version__
-                ) + " | Frame {0}\n".format(ts.frame)
+            remark = "frame {} | Written by MDAnalysis {} (release {})\n".format(
+                ts.frame, self.__class__.__name__, __version__)
 
             self._xyz.write(remark)
         else:
diff --git a/testsuite/MDAnalysisTests/coordinates/test_xyz.py b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
index 1a8246f715c..bdc7d10c043 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_xyz.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
@@ -111,14 +111,10 @@ def test_no_conversion(self, ref, reader, tmpdir):
         [   
             2 * ["Curstom Remark"],
             2 * [""],
-            [None, "Written by MDAnalysis XYZWriter (release {0}) | Frame 0".format(__version__)],
+            [None, "frame 0 | Written by MDAnalysis XYZWriter (release {0})".format(__version__)],
         ]
     )
     def test_remark(self, remarkout, remarkin, ref, tmpdir):
-        """
-        Test remark handling.
-        """
-
         u = mda.Universe(ref.topology, ref.trajectory)
 
         outfile = "write-remark.xyz"

From 5ef97b50c1fe440478fc86cacda5ecce58258caa Mon Sep 17 00:00:00 2001
From: Rocco Meli <r.meli@bluemail.ch>
Date: Sun, 7 Jun 2020 01:34:24 +0200
Subject: [PATCH 17/17] suggested change

Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
---
 testsuite/MDAnalysisTests/coordinates/test_xyz.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/testsuite/MDAnalysisTests/coordinates/test_xyz.py b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
index bdc7d10c043..d4f9819ec4b 100644
--- a/testsuite/MDAnalysisTests/coordinates/test_xyz.py
+++ b/testsuite/MDAnalysisTests/coordinates/test_xyz.py
@@ -116,7 +116,6 @@ def test_no_conversion(self, ref, reader, tmpdir):
     )
     def test_remark(self, remarkout, remarkin, ref, tmpdir):
         u = mda.Universe(ref.topology, ref.trajectory)
-
         outfile = "write-remark.xyz"
 
         with tmpdir.as_cwd():