Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
7af3352
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Aug 7, 2019
51624f3
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Aug 9, 2019
fa7777e
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Aug 13, 2019
94ac6cc
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Aug 20, 2019
80370af
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Sep 5, 2019
36fa2ea
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Sep 10, 2019
5111103
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Oct 2, 2019
ad0c149
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Oct 3, 2019
def7c14
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Oct 16, 2019
2d693e3
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Oct 30, 2019
f7d5bc6
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Nov 21, 2019
64bcae5
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Nov 28, 2019
2e2561e
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Dec 23, 2019
2184e2b
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 14, 2020
de7afcc
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 16, 2020
ff4ed8d
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 17, 2020
c515e89
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 17, 2020
7d27138
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 18, 2020
3c35af0
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 21, 2020
22def6a
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 23, 2020
93413dc
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Jan 24, 2020
8c5dc00
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Feb 1, 2020
ad1766e
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Feb 4, 2020
65e4d66
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Feb 4, 2020
549c1a2
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Feb 6, 2020
f527e62
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Feb 9, 2020
4d5f7de
Merge remote-tracking branch 'upstream/develop' into develop
RMeli Feb 9, 2020
1b4c29f
failing test
RMeli Feb 9, 2020
62b18d9
check if file is StringIO
RMeli Feb 9, 2020
432ed4e
changelog
RMeli Feb 9, 2020
70f2971
small change
RMeli Feb 9, 2020
7850ff3
fix typos
RMeli Feb 9, 2020
e4520f8
actually check exception message
RMeli Feb 9, 2020
02c7eb4
fix problem with exception catch
RMeli Feb 9, 2020
6efe65a
use pytest match
RMeli Feb 10, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ mm/dd/yy richardjgowers, kain88-de, lilyminium, p-j-smith, bdice, joaomcteixeira
* 0.21.0

Fixes
* Handle exception when PDBWriter is trying to remove an invalid StringIO
(Issue #2512)
* Clarifies density_from_Universe docs and adds user warning (Issue #2372)
* Fix upstream deprecation of `matplotlib.axis.Tick` attributes in
`MDAnalysis.analysis.psa`
Expand Down
12 changes: 12 additions & 0 deletions package/MDAnalysis/coordinates/PDB.py
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,10 @@ def _check_pdb_coordinates(self):
coordinates and closes the file.

Raises :exc:`ValueError` if the coordinates fail the check.

.. versionchanged: 1.0.0
Check if :attr:`filename` is `StringIO` when attempting to remove
a PDB file with invalid coordinates (Issue #2512)
"""
atoms = self.obj.atoms # make sure to use atoms (Issue 46)
# can write from selection == Universe (Issue 49)
Expand All @@ -678,6 +682,14 @@ def _check_pdb_coordinates(self):
except OSError as err:
if err.errno == errno.ENOENT:
pass
else:
raise
except TypeError:
if isinstance(self.filename, StringIO):
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equivalent to

except TypeError:
    pass

Shouldn't it be the following?

except TypeError:
    if isinstance(self.filename, StringIO):
        pass
    else:
        raise

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely! Thanks for spotting this problem!

Copy link
Member Author

@RMeli RMeli Feb 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same logic should apply to the previous exception as well, which is suffering from the same problem.

However, this rises a question: should we ignore all the exceptions that can occur while trying os.remove so that the correct exception ValueError (which is the problem triggering os.remove) is thrown instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can at least wonder what would be the equivalent of an os.remove on a StringIO. Now, we have an odd discrepancy in behaviour between files and StringIO. Maybe it should be a different issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the equivalent of os.remove for StringIO is just getting out of scope. However, it looks like that calling close() already frees the memory buffer:

StringIO.close()
Free the memory buffer. Attempting to do further operations with a closed StringIO object will raise a ValueError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, then, that the handling is appropriate.

else:
raise

raise ValueError("PDB files must have coordinate values between "
"{0:.3f} and {1:.3f} Angstroem: file writing was "
"aborted.".format(self.pdb_coor_limits["min"],
Expand Down
24 changes: 24 additions & 0 deletions testsuite/MDAnalysisTests/coordinates/test_pdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,10 @@ def universe(self):
def universe2(self):
return mda.Universe(PSF, DCD)

@pytest.fixture
def universe3(self):
return mda.Universe(PDB)

@pytest.fixture
def outfile(self, tmpdir):
return str(tmpdir.mkdir("PDBWriter").join('primitive-pdb-writer' + self.ext))
Expand Down Expand Up @@ -362,6 +366,26 @@ def test_segid_chainid(self, universe2, outfile):
u_pdb = mda.Universe(outfile)
assert u_pdb.segments.chainIDs[0][0] == ref_id

def test_stringio_outofrange(self, universe3):
"""
Check that when StringIO is used, the correct out-of-range error for
coordinates is raised (instead of failing trying to remove StringIO
as a file).
"""

u = universe3

u.atoms.translate([-9999, -9999, -9999])

outstring = StringIO()

errmsg = "PDB files must have coordinate values between"

with pytest.raises(ValueError, match=errmsg):
with mda.coordinates.PDB.PDBWriter(outstring) as writer:
writer.write(u.atoms)


class TestMultiPDBReader(object):
@staticmethod
@pytest.fixture(scope='class')
Expand Down