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

Implements Py3-compatible rich comparison for all Bio.PDB objects #1360

Merged
merged 10 commits into from Aug 15, 2017

Conversation

JoaoRodrigues
Copy link
Member

This pull request addresses issue #1356 and refactors #1189 and PR #1291. It implements rich comparisons for every object of the SMCRA hierarchy, including disordered wrappers. This allows direct comparison of any two objects and allows calling the sorted method directly, unifying the API.

The test suite was expanded and refactored to test all these possibilities. The a_structure.pdb test PDB was slightly tweaked to avoid adding yet another file. I had to fix a number of other tests to accomodate these changes. All tests pass under Python 2.7 and Python 3.

Thanks @harryjubb for raising the problem that led to this fix, and @peterjc for the earlier work and usual leadership 👍

I hereby agree to dual licence this and any previous contributions under both
the Biopython License Agreement AND the BSD 3-Clause License.

I am happy to be thanked by name in the NEWS.rst and
CONTRIB.rst files.

I have read the CONTRIBUTING.rst file and understand that AppVeyor and
TravisCI will be used to confirm the Biopython unit tests and flake8 style
checks pass with these changes.

…RA objects. Special rules for Chains, Residues, and Atoms
… object sorting. Edited other tests to comply with changes.

self.child_list.sort(key=sort_index)

self.child_list.sort
Copy link
Member

Choose a reason for hiding this comment

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

Should that not be self.child_list.sort() for an in-place sorting of the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted!

@peterjc
Copy link
Member

peterjc commented Aug 9, 2017

Is it worth adding public .sort() methods to each level of objects? i.e. Where there used to be a private ._sort() method until #1189

@peterjc
Copy link
Member

peterjc commented Aug 9, 2017

Are you happy with running flake8 locally with the plugins as described in the CONTRIBUTING file? The TravisCI style checks are failing right now:

$ flake8 Bio/
Bio/PDB/Residue.py:92:1: D202 No blank lines allowed after function docstring

After that it will run flake8 Tests/ as well.

@JoaoRodrigues
Copy link
Member Author

JoaoRodrigues commented Aug 9, 2017

Is it worth adding public .sort() methods to each level of objects? i.e. Where there used to be a private ._sort() method until #1189

I would actually propose removing them altogether and have people use the sorted method instead. This is already supported anyway.

Are you happy with running flake8 locally with the plugins as described in the CONTRIBUTING file? The TravisCI style checks are failing right now

Yes. I'll do that. I had a linter plugin for Atom (the editor) but that kept leaving Python processes hanging after I closed the program (100% CPU for hours...). I'll run locally. Do we have a pre-commit hook for this somewhere? Would be nice to have at hand...

@peterjc
Copy link
Member

peterjc commented Aug 9, 2017

OK - should we deprecate the single public .sort() method then?

For the git pre-commit hook, see issue #493 and possible implementation on #1188

return (i, atom.name, atom.altloc)

self.child_list.sort(key=sort_index)

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to remove this blank link to make flake8 happy:

$ flake8 Bio/
Bio/PDB/Residue.py:92:1: D202 No blank lines allowed after function docstring

@JoaoRodrigues
Copy link
Member Author

OK - should we deprecate the single public .sort() method then?

I would vote for that.

@JoaoRodrigues
Copy link
Member Author

JoaoRodrigues commented Aug 9, 2017

@etal @peterjc Some tests are failing on Py 3.4 and made me realize I might have misunderstood how to write these methods. For example, for an __eq__ method for equality, should I explicitly write what I expect to compare in the other object - i.e. return self.id == other.id - or should I leave it to the language and implement it on both objects like return self.id == other? It seems that the latter is the best choice because it tries to figure out how to compare the objects first and if it cannot, then it raises the normal error 'Cannot compare objects x and y' instead of a missing attribute in case the other object is say, a list. Any thoughts? Object equality is not really taught in biology classes :)

Edit: Added a test for isinstance(other, type(self)) to avoid these problems, as suggested in several places. Trying a comparison between any of our objects and anything else returns NotImplemented if they are not of the same type, which then defaults to whatever Python decides (equality based on id comparison, TypeError for >.>=,<, and <=).

@codecov
Copy link

codecov bot commented Aug 9, 2017

Codecov Report

Merging #1360 into master will decrease coverage by 0.12%.
The diff coverage is 46.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1360      +/-   ##
==========================================
- Coverage   84.48%   84.36%   -0.13%     
==========================================
  Files         319      319              
  Lines       48920    49100     +180     
==========================================
+ Hits        41332    41423      +91     
- Misses       7588     7677      +89
Impacted Files Coverage Δ
Bio/PDB/Chain.py 63.38% <28.12%> (-28.93%) ⬇️
Bio/PDB/Residue.py 68.9% <42.85%> (-18%) ⬇️
Bio/PDB/Atom.py 76.21% <50%> (-10.81%) ⬇️
Bio/PDB/Entity.py 83.24% <61.76%> (-4.84%) ⬇️
Bio/PDB/MMCIF2Dict.py 100% <0%> (ø) ⬆️

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 32ca898...fd356bc. Read the comment docs.

@@ -108,6 +108,10 @@ def sort(self):
by name, with any alternative location specifier for disordered
atoms (altloc) as a tie-breaker.
"""
warnings.warn("The custom sort() method will be deprecated in the "
Copy link
Member

Choose a reason for hiding this comment

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

change "deprecated" to "removed" (or change the tense to "... has been deprecated in favour of...")

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@JoaoRodrigues
Copy link
Member Author

The coverage is decreasing because of all the rich comparison methods, which have a few conditionals in there, but the testing is actually more extensive than before. If others agree with these changes, we should merge this.

@peterjc
Copy link
Member

peterjc commented Aug 10, 2017

Seems fine to me - @etal, @lennax, etc any comments before we merge this?

@peterjc peterjc merged commit eca476a into biopython:master Aug 15, 2017
peterjc added a commit that referenced this pull request Aug 15, 2017
See preceeding commits from pull request #1360

[ci skip]
MarkusPiotrowski pushed a commit to MarkusPiotrowski/biopython that referenced this pull request Oct 31, 2017
See preceeding commits from pull request biopython#1360

[ci skip]
@JoaoRodrigues JoaoRodrigues deleted the ns_fix branch June 26, 2020 20:40
sarah-robinson pushed a commit to sarah-robinson/biopython that referenced this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants