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

Added methods to remove children of disordered Entities #3266

Merged
merged 5 commits into from
Sep 14, 2020

Conversation

JoaoRodrigues
Copy link
Member

@JoaoRodrigues JoaoRodrigues commented Sep 9, 2020

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

  • I have read the CONTRIBUTING.rst file, have run pre-commit locally,
    and understand that AppVeyor and TravisCI will be used to confirm the Biopython unit
    tests and style checks pass with these changes.

  • I have added my name to the alphabetical contributors listings in the files
    NEWS.rst and CONTRIB.rst as part of this pull request, am listed
    already, or do not wish to be listed. (This acknowledgement is optional.)

Disordered entities like DisorderedAtom and DisorderedResidues allow adding children to them but not removing. Removing is useful when editing structures. This PR adds methods to allow that to happen and simplifies, for instance, removing altlocs from a structure:

s = ...  # structure
for atom in s.get_atoms():
    if atom.is_disordered():
        altlocs = list(atom.child_dict)
        altlocs.remove(atom.selected_child)
        for a in altlocs:
            atom.disordered_remove(a)

The methods do not remove the disordered entity completely even if it is empty. I thought that would be better left to the user to do it explicitly from the parent. They do change the __repr__ of the disordered entity to signal that they are empty.

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #3266 into master will decrease coverage by 0.00%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3266      +/-   ##
==========================================
- Coverage   83.93%   83.93%   -0.01%     
==========================================
  Files         317      317              
  Lines       51556    51584      +28     
==========================================
+ Hits        43275    43297      +22     
- Misses       8281     8287       +6     
Impacted Files Coverage Δ
Bio/PDB/Entity.py 92.24% <50.00%> (-0.37%) ⬇️
Bio/PDB/Residue.py 82.22% <62.50%> (+0.17%) ⬆️
Bio/PDB/Atom.py 89.22% <82.35%> (-0.69%) ⬇️

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 249b767...67fb0a5. Read the comment docs.

@peterjc
Copy link
Member

peterjc commented Sep 9, 2020

Are there pre-existing similar methods on the other objects for removing children? How would you remove a normal atom from a chain for example?

@JoaoRodrigues
Copy link
Member Author

JoaoRodrigues commented Sep 9, 2020

There are. Entity has detach_child() that works both directly and via __delitem__. So you can remove an atom from a residue by doing del residue["CA"] for instance, or an entire chain from a model with del model["A"].

I used disordered_remove since there is a disordered_add method. Seems the clearest.

@Lucioric2000
Copy link

I think that in the above example, the calling of altlocs.remove(atom.selected_child) is unnecesary, because altlocs is only a list not stated to be used afterwards. The object tree edition is done in the for cycle.

@JoaoRodrigues
Copy link
Member Author

I think that in the above example, the calling of altlocs.remove(atom.selected_child) is unnecesary, because altlocs is only a list not stated to be used afterwards. The object tree edition is done in the for cycle.

I had a tiny typo in the for. The idea was to show how to remove all other altlocs but the primary one, thus the remove. Is the syntax clear for you?

@peterjc
Copy link
Member

peterjc commented Sep 9, 2020

Thanks for clarifying the thoughts behind the method naming. That makes sense. I think that's all I can usefully offer for a review - who else would you suggest for a more informed second opinion? Maybe @etal?

@JoaoRodrigues
Copy link
Member Author

Without wanting to bother him too much (I've been pestering both of you lately with all these PRs...), @jgreener64 would be a good reviewer if @etal is not available!

@jgreener64
Copy link
Contributor

I'm not so familiar with the ways to modify entities or the internals of the disorder code, so am not sure I'll be able to thoroughly review this. Seems okay from a quick look though.

On a separate note, possibly for another PR, it would be good to add some material to the docs on adding or removing entities to structure objects.

@JoaoRodrigues
Copy link
Member Author

Thanks @jgreener64 !

Indeed. I was thinking about that this weekend - at some point in the past we discussed moving the tutorials to Jupyter notebooks. I remember @tiagoantao was leading this and indeed, there's a repo. Given the pervasiveness of conda and jupyter notebooks nowadays, I would be tempted to revist this approach to illustrate a bunch of examples (basic and not so basic) of our codebase. @peterjc , there are two folders in Docs, cookbook and examples. Could we maybe consider merging these two in a single folder (either called examples or cookbook, I like the latter) and moving the examples to notebook format?

Related: the "Going 3D" section of the tutorial could use a little reorganization and update.

@JoaoRodrigues
Copy link
Member Author

Meanwhile, I will merge this. I tested this branch on a few thousand PDB files (~8000) and there seem to be no outstanding issues.

@JoaoRodrigues JoaoRodrigues merged commit 3b9e624 into biopython:master Sep 14, 2020
@JoaoRodrigues JoaoRodrigues deleted the disordered_remove branch September 14, 2020 22:00
@peterjc
Copy link
Member

peterjc commented Sep 15, 2020

I think (without checking the history) that the cookbook folder was superceded by the cookbook category on the then wiki based website: https://biopython.org/wiki/Category:Cookbook - I think Doc/cookbook could be retired, note we've talked about deprecating the restriction code anyway. This would also solve the confusion with two things called cookbook.

@JoaoRodrigues
Copy link
Member Author

Thanks. What do you think about moving these (including the recipes on the wiki/website) to a notebook format and storing them in the main source tree?

@peterjc
Copy link
Member

peterjc commented Sep 16, 2020

Does it have to be the same repository?

Could we can run automated continuous integration testing on the notebooks? Static documentation without tests came become out of date surprisingly often (and things like doctests and testing examples in the tutorial often catches gaps in our main test coverage).

@JoaoRodrigues
Copy link
Member Author

Nope, it can be a separate one. We can run CI on the notebooks, I did exactly the same for another repo of mine. I'll start with Bio.PDB and then we can see how to proceed? Shall I make a repo myself and maybe later transfer it or create one directly under the biopython org?

@peterjc
Copy link
Member

peterjc commented Sep 16, 2020

If you want to make one under https://github.com/biopython/ that sounds sensible to me, but by all means experiment first on your own repository first if you'd rather.

@peterjc
Copy link
Member

peterjc commented Sep 16, 2020

Having notebooks in a separate repository would make it easy to test both the latest Biopython master branch and one or more recent releases - that would be a plus.

@JoaoRodrigues
Copy link
Member Author

👍 Thank you! I'll give it a shot then on my own account first not to pollute the main account :)

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.

4 participants