updated to return Permissive PDB Reader and Parser for all PDBs #812

jdetle · 2016-04-02T22:57:48Z

Fixes #777

Changes made in this Pull Request:

Changed all relevant doc strings to eliminate references to Biopython Readers/Writers and parsers
Removed the Biopython sloppy structure builder, (entire ~/MDAnalysis/coordinates/pdb directory)
Replaced PDBReader, PDBWriter, PDBParser with their 'Primitive' counterparts
Kept 'Primitive' PDBReader, Writer and Parser that inherit from their PDB counterparts and have no format name in the way that the PDBReader has format='PDB', added deprecation warnings indicating this change
Changed PDB tests to remove known failures of Biopython
Updated doc strings to remove references to Primitive PDB Readers, Writers and Parsers.

PR Checklist

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

jdetle · 2016-04-02T23:08:03Z

The travis CI build should fail at this point, tests requiring the PDB reader need to be removed or made into a known fail. If this looks good I'll go ahead and remove the tests involving the strict PDB reader. In addition, by my understanding, the QC issue probably should be ignored. If we want a permissive/strict reader distinction for some other format at some point down the road this backbone will be useful.

jdetle · 2016-04-03T00:38:29Z

I'm thinking now that it would be better to just go ahead and delete all references to the BioPython PDB Reader and Parser altogether. Would this mean that we just go ahead and rename the PrimitivePDBReader to be the PDBReader?

kain88-de · 2016-04-03T08:58:59Z

Would this mean that we just go ahead and rename the PrimitivePDBReader to be the PDBReader?

Yes you can do that. Please still keep a dummy PrimitivePDBReader object with a deprecation notice around so ease the transition for users who work directly with the PrimitiveReader object. It should work something like this. But I'm not 100% sure.

@deprecated(...)
PrimitivePDBReader = PDBReader

kain88-de · 2016-04-03T09:00:07Z

BTW you also need to change the documentation what the BioPython parsers are removed and check that the tests still pass, so you can't cross out the two points.

jdetle · 2016-04-03T23:33:19Z

This is very clearly broken as of right now, I am seeing issues with import errors that I will address either tonight or tomorrow. Fixing the tests shouldn't be difficult after the code is working.

as options for 'permissive'=False. Added deprecation warnings for Primitive Readers/Writers and Parsers. Changed doc strings to eliminate references to BioPython Reader/Writer. Updated CHANGELOG to reflect changes.

jdetle · 2016-04-04T20:24:23Z

This is the only failure.

FAIL: test_writer (MDAnalysisTests.coordinates.test_pdbqt.TestPDBQT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jdetlefs/github/MDAnalysis/testsuite/MDAnalysisTests/coordinates/test_pdbqt.py", line 76, in test_writer
    "Writing and reading of chain A does not recover same atoms")
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 255, in assert_equal
    assert_equal(actual[k], desired[k], 'item=%r\n%s' % (k, err_msg), verbose)
  File "/usr/lib/python2.7/dist-packages/numpy/testing/utils.py", line 317, in assert_equal
    raise AssertionError(msg)
AssertionError: 
Items are not equal:
item=0
Writing and reading of chain A does not recover same atoms
 ACTUAL: <Atom 1: N of type 620 of resname PRO, resid 2 and segid A>
 DESIRED: <Atom 1: N of type N of resname PRO, resid 2 and segid A>
-------------------- >> begin captured logging << --------------------
MDAnalysis.core.AtomGroup: DEBUG: Universe.load_new(): loading /home/jdetlefs/github/MDAnalysis/testsuite/MDAnalysisTests/data/pdbqt_inputpdbqt.pdbqt...
MDAnalysis.core.AtomGroup: DEBUG: Universe.load_new(): loading /home/jdetlefs/github/MDAnalysis/testsuite/MDAnalysisTests/data/pdbqt_querypdb.pdb...
MDAnalysis.core.AtomGroup: DEBUG: Universe.load_new(): loading /tmp/tmpXLkwj6/test.pdbqt...

I'm getting an error for a PDBQT test, which is strange since I didn't change the file at all and it doesn't have any dependencies on the PDBReader, Writer or Parser, does anyone know of this problem?

FWIW its worth this seems to only be a problem on my machine, and fails in my copy of the upstream/develop branch as well.

jdetle · 2016-04-05T21:31:40Z

I just did some testing and realized I need to move the deprecation warnings to inside the objects. I'll get on this tonight.

caused by BioPython, updated CHANGELOG.

jbarnoud · 2016-04-06T06:46:12Z

Out of curiosity, why using warnings rather than the deprecation warning? (I have no idea on the constraints of the decorator)

jdetle · 2016-04-06T16:31:31Z

The deprecate decorator wasn't working. I didn't actually try it in the form of @deprecate(old_name='',new_name='', message='') but I did a very quick search through the code and warnings seemed like a commonly used tool so I used that instead. I can go ahead and give what I said earlier a try if there is a preferred style.

kain88-de · 2016-04-11T17:20:43Z

package/MDAnalysis/core/AtomGroup.py

@@ -3960,6 +3960,9 @@ def __init__(self, *args, **kwargs):
        .. versionchanged:: 0.11.0
           Added the *is_anchor* and *anchor_name* keywords for finer behavior
           control when unpickling instances of :class:`MDAnalysis.core.AtomGroup.AtomGroup`.
+        .. versionchanged:: 0.14.1


wrong version. It will be 0.15.0

kain88-de · 2016-04-11T17:24:00Z

@jdetle did you make any progress on this? There are some conflicts with the develop branch you have to solve.

All in all this goes into a good direction. I haven't noticed to much looking over the changes right now.

jdetle · 2016-04-11T17:26:17Z

I thought that keeping the permissive flag for a version would help the transition. If this is bad practice I can go ahead and delete it. Right now it really does nothing so I can see now why this would be bad practice. The conflict is only in the changelog. I'll go ahead and fix that now.

jdetle · 2016-04-11T17:58:26Z

For some reason I can't see commit e27c33a when rebasing. I vote that we keep the permissive argument to avoid confusing people until they understand the feature is going away. If we didn't keep it, why do we have a dummy primitive readers/writers/parsers?

jbarnoud · 2016-04-11T18:02:08Z

I also think we should keep it with a warning during the deprecation period.

On 11-04-16 19:58, John Detlefs wrote:

For some reason I can't see commit e27c33a
e27c33a
when rebasing. I vote that we keep the permissive argument to avoid
confusing people until they understand the feature is going away. If
we didn't keep it, why do we have a dummy primitive
readers/writers/parsers?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#812 (comment)

jdetle · 2016-04-16T22:45:54Z

Okay, changes made. Should I rebase to one big commit?

orbeckst · 2016-04-16T23:08:24Z

It seems to me that your commits are sensible self-contained units but maybe someone else has stronger feelings about this.

jdetle · 2016-04-16T23:09:57Z

Okay, actually I forgot the sloppy builder so I still need to get that.

…ment. Updated tests in test_pdb and test_topology, updated tests to remove references to the permissive flag. Updated docs to identify deprecated modules, removed structure builder and updated the CHANGELOG

jdetle · 2016-04-18T18:18:11Z

Build passing, need to update the changelog, but I made the changes that you asked for @orbeckst, @kain88-de

orbeckst · 2016-04-18T19:07:46Z

package/MDAnalysis/coordinates/core.py

@@ -67,8 +64,8 @@ def get_reader_for(filename, permissive=False, format=None):
    if format is None:
        format = util.guess_format(filename)
    format = format.upper()
-    if permissive and format == 'PDB':
-        return _READERS['Permissive_PDB']
+    if format == 'PDB':


This whole statement should be removed. (Lines 67 & 68) because we'll just run through the try/except below.

orbeckst · 2016-04-18T19:17:10Z

There are still occurrences of

format = "Permissive_PDB"

We should not need this anymore. Just change them all to "PDB" instead of "Permissive_PDB".

jdetle · 2016-04-18T19:39:43Z

Alright! Will do, thanks.
On Mon, Apr 18, 2016 at 12:17 PM Oliver Beckstein [email protected]
wrote:

There are still occurrences of

format = "Permissive_PDB"

We should not need this anymore. Just change them all to "PDB" instead of
"Permissive_PDB".

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#812 (comment)

Have a wonderful day,
John J. Detlefs

jdetle · 2016-04-20T00:25:00Z

@orbeckst After thinking about this a little bit I remembered why I was doing what I was doing. By my understanding the point of the deprecation warning in the primitive readers/writers/parsers was to allow a user to pass format='Permissive_PDB' to the get_reader_for() function and have it return the (dummy/placeholder) primitive pdb reader with the warning. If we are getting rid of all references to format='Permissive_PDB', then we might as well go ahead and delete the dummy Primitive Readers/Writers and Parsers because there is no way they can be retrieved by the user.

orbeckst · 2016-04-20T00:34:56Z

On 19 Apr, 2016, at 17:25, John Detlefs [email protected] wrote:

@orbeckst After thinking about this a little bit I remembered why I was doing what I was doing. By my understanding the point of the deprecation warning in the primitive readers/writers/parsers was to allow a user to pass a permissive_pdb format to the get_reader_for() function and have it return the (dummy/placeholder) primitive pdb reader with the warning. If we are getting rid of all references to the format='Permissive_PDB', then we might as well go ahead and delete the dummy Primitive Readers/Writers and Parsers because there is no way they can be retrieved by the user.

One can still use the class directly in user code. Hopefully unlikely but possible. Let them sit there for one release cycle and then we remove them as scheduled.

(Deprecations are tricky business because you never know how other people use your code. But all the classes are officially documented as the API and thus users are welcome to use them directly, e.g. someone might have subclassed PrimitivePDBReader for their own purposes. We don’t know this so we have to honor the promise that we make through our API documentation and at least give them some warning.)

Oliver Beckstein * [email protected]
skype: orbeckst * [email protected]

jdetle · 2016-04-20T00:57:42Z

Okay, that works, thanks.
On Tue, Apr 19, 2016 at 5:35 PM Oliver Beckstein [email protected]
wrote:

On 19 Apr, 2016, at 17:25, John Detlefs [email protected]
wrote:

@orbeckst After thinking about this a little bit I remembered why I was
doing what I was doing. By my understanding the point of the deprecation
warning in the primitive readers/writers/parsers was to allow a user to
pass a permissive_pdb format to the get_reader_for() function and have it
return the (dummy/placeholder) primitive pdb reader with the warning. If we
are getting rid of all references to the format='Permissive_PDB', then we
might as well go ahead and delete the dummy Primitive Readers/Writers and
Parsers because there is no way they can be retrieved by the user.

One can still use the class directly in user code. Hopefully unlikely but
possible. Let them sit there for one release cycle and then we remove them
as scheduled.

(Deprecations are tricky business because you never know how other people
use your code. But all the classes are officially documented as the API and
thus users are welcome to use them directly, e.g. someone might have
subclassed PrimitivePDBReader for their own purposes. We don’t know this so
we have to honor the promise that we make through our API documentation and
at least give them some warning.)

Oliver Beckstein * [email protected]
skype: orbeckst * [email protected]

—

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#812 (comment)

Have a wonderful day,
John J. Detlefs

richardjgowers · 2016-04-20T14:52:30Z

testsuite/MDAnalysisTests/coordinates/test_pdb.py

-                            err_msg="Writing PDB file with PrimitivePDBWriter "
-                            "does not reproduce original coordinates")
+        u = mda.Universe(PSF, self.outfile)
+        assert_almost_equal(u.atoms.coordinates(),


Why has this gone back to coordinates()? Is it because we changed this to positions after you created this branch?

Resolved the conflict wrong while tired last night, my apologies!

richardjgowers · 2016-04-20T14:54:58Z

Looking good, just fix the coordinates() to positions again. It's nice to simplify to a single PDB solution!

orbeckst · 2016-04-22T02:56:39Z

@jdetle I created PR #832 to finish the merge. I am using #832 to do the merging and cleanup. I will therefore close this PR and continue at #832.

jdetle · 2016-04-22T03:50:55Z

Thanks Oliver! I was having some trouble rebasing commits made before some
merges so I appreciate you taking on some of the work, have a good night.
On Thu, Apr 21, 2016 at 7:56 PM Oliver Beckstein [email protected]
wrote:

Closed #812 #812.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#812 (comment)

Have a wonderful day,
John J. Detlefs

jdetle force-pushed the develop branch from 8857201 to c3413ff Compare April 3, 2016 23:32

jdetle force-pushed the develop branch 2 times, most recently from 8b34f1b to 5d51914 Compare April 4, 2016 19:54

Eliminated Biopython PDB Reader/Writer and Parser

744e644

as options for 'permissive'=False. Added deprecation warnings for Primitive Readers/Writers and Parsers. Changed doc strings to eliminate references to BioPython Reader/Writer. Updated CHANGELOG to reflect changes.

jdetle force-pushed the develop branch from 5d51914 to 744e644 Compare April 4, 2016 19:57

Changed tests to eliminate known failures

2ee04ba

caused by BioPython, updated CHANGELOG.

jdetle force-pushed the develop branch 3 times, most recently from 492add3 to 64dfdfa Compare April 6, 2016 04:47

kain88-de reviewed Apr 11, 2016
View reviewed changes

jdetle force-pushed the develop branch from 57e8c57 to cb50838 Compare April 11, 2016 18:29

Merged to resolve conflicts from pull requests merged previously

fdb4f52

jdetle force-pushed the develop branch 2 times, most recently from 9b67e9d to b82e11e Compare April 11, 2016 18:43

Removed references to permissive flag in docs, and its use as an argu…

f8ff898

…ment. Updated tests in test_pdb and test_topology, updated tests to remove references to the permissive flag. Updated docs to identify deprecated modules, removed structure builder and updated the CHANGELOG

jdetle force-pushed the develop branch from ff99972 to f8ff898 Compare April 16, 2016 23:46

orbeckst reviewed Apr 18, 2016
View reviewed changes

jdetle added 2 commits April 19, 2016 21:49

Removed references to Permissive_PDB

94980b7

Fixed merge conflicts and updated CHANGELOG

82fb36b

richardjgowers reviewed Apr 20, 2016
View reviewed changes

richardjgowers added this to the 0.15.0 milestone Apr 20, 2016

jdetle force-pushed the develop branch from b19baf2 to 626a7f2 Compare April 20, 2016 17:02

Updated CHANGELOG, fixed errors in tests, updated docs

3fac983

jdetle force-pushed the develop branch from 626a7f2 to 3fac983 Compare April 21, 2016 00:13

orbeckst added maintainability Format-PDB Component-Readers refactoring labels Apr 22, 2016

orbeckst mentioned this pull request Apr 22, 2016

remove Bio.PDBParser (Issue #777) #832

Merged

4 tasks

orbeckst closed this Apr 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated to return Permissive PDB Reader and Parser for all PDBs #812

updated to return Permissive PDB Reader and Parser for all PDBs #812

jdetle commented Apr 2, 2016 •

edited

Loading

jdetle commented Apr 2, 2016

jdetle commented Apr 3, 2016

kain88-de commented Apr 3, 2016

kain88-de commented Apr 3, 2016

jdetle commented Apr 3, 2016

jdetle commented Apr 4, 2016

jdetle commented Apr 5, 2016

jbarnoud commented Apr 6, 2016

jdetle commented Apr 6, 2016 •

edited

Loading

kain88-de Apr 11, 2016

kain88-de commented Apr 11, 2016

jdetle commented Apr 11, 2016

jdetle commented Apr 11, 2016

jbarnoud commented Apr 11, 2016

jdetle commented Apr 16, 2016

orbeckst commented Apr 16, 2016

jdetle commented Apr 16, 2016

jdetle commented Apr 18, 2016

orbeckst Apr 18, 2016

orbeckst commented Apr 18, 2016

jdetle commented Apr 18, 2016

jdetle commented Apr 20, 2016 •

edited

Loading

orbeckst commented Apr 20, 2016

jdetle commented Apr 20, 2016

richardjgowers Apr 20, 2016

jdetle Apr 20, 2016

richardjgowers commented Apr 20, 2016

orbeckst commented Apr 22, 2016

jdetle commented Apr 22, 2016

updated to return Permissive PDB Reader and Parser for all PDBs #812

updated to return Permissive PDB Reader and Parser for all PDBs #812

Conversation

jdetle commented Apr 2, 2016 • edited Loading

PR Checklist

jdetle commented Apr 2, 2016

jdetle commented Apr 3, 2016

kain88-de commented Apr 3, 2016

kain88-de commented Apr 3, 2016

jdetle commented Apr 3, 2016

jdetle commented Apr 4, 2016

jdetle commented Apr 5, 2016

jbarnoud commented Apr 6, 2016

jdetle commented Apr 6, 2016 • edited Loading

kain88-de Apr 11, 2016

Choose a reason for hiding this comment

kain88-de commented Apr 11, 2016

jdetle commented Apr 11, 2016

jdetle commented Apr 11, 2016

jbarnoud commented Apr 11, 2016

jdetle commented Apr 16, 2016

orbeckst commented Apr 16, 2016

jdetle commented Apr 16, 2016

jdetle commented Apr 18, 2016

orbeckst Apr 18, 2016

Choose a reason for hiding this comment

orbeckst commented Apr 18, 2016

jdetle commented Apr 18, 2016

jdetle commented Apr 20, 2016 • edited Loading

orbeckst commented Apr 20, 2016

jdetle commented Apr 20, 2016

richardjgowers Apr 20, 2016

Choose a reason for hiding this comment

jdetle Apr 20, 2016

Choose a reason for hiding this comment

richardjgowers commented Apr 20, 2016

orbeckst commented Apr 22, 2016

jdetle commented Apr 22, 2016

jdetle commented Apr 2, 2016 •

edited

Loading

jdetle commented Apr 6, 2016 •

edited

Loading

jdetle commented Apr 20, 2016 •

edited

Loading