Select dihedrals faster #2706

lilyminium · 2020-06-01T11:21:33Z

Fixes (part of) #2671

Changes made in this Pull Request:

made phi/psi_selection longer, but faster with manual boolean array matching of atom names
made Ramachandran really long and ugly, but really faster with manual boolean array matching to atom names
no longer hardcodes phi/psi selection names in Ramachandran

The original Ramachandran.__init__ with the GRO/XTC files was 180 s on my laptop. When I changed only the phi_selection code (not psi_selection), it dropped to 122 s. When I changed the Ramachandran code, it dropped to 0.25 s. Most of that is selecting the protein.

~~How much testing does this need?~~ lots

PR Checklist

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

To do

update omega_selection and chi1_selection too
add c_name, n_name etc. as kwargs to those, it's annoying that the names are hard-coded in (edit: will remove pending disapproval from another dev)
add the group selection methods to ResidueGroup
update Ramachandran.__init__ to use the group selection methods

Edit: Ramachandran is no longer in the slowest 50 tests.

========================== slowest 50 test durations ===========================

39.76s call     MDAnalysisTests/analysis/test_density.py::Test_density_from_Universe::test_notwithin

31.18s call     MDAnalysisTests/analysis/test_density.py::TestNotWithin::test_within

30.83s call     MDAnalysisTests/analysis/test_density.py::TestNotWithin::test_not_within

29.70s call     MDAnalysisTests/analysis/test_density.py::TestNotWithin::test_has_DeprecationWarning

29.58s call     MDAnalysisTests/analysis/test_hbonds.py::TestHydrogenBondAnalysis::test_true_traj

29.21s call     MDAnalysisTests/analysis/test_hbonds.py::TestHydrogenBondAnalysisPBC::test_true_traj

29.15s call     MDAnalysisTests/analysis/test_hbonds.py::TestHydrogenBondAnalysisHeuristic::test_true_traj

29.11s call     MDAnalysisTests/analysis/test_hbonds.py::TestHydrogenBondAnalysisHeavy::test_true_traj

29.11s call     MDAnalysisTests/analysis/test_hbonds.py::TestHydrogenBondAnalysisHeavyFail::test_true_traj

21.78s call     MDAnalysisTests/visualization/test_streamlines.py::test_streamplot_2D

14.44s call     MDAnalysisTests/analysis/test_hole2.py::TestHoleAnalysis::test_context_manager

12.24s call     MDAnalysisTests/coordinates/test_pdb.py::test_conect_bonds_all

10.71s call     MDAnalysisTests/analysis/test_density.py::Test_density_from_Universe::test_update_selection

9.67s call     MDAnalysisTests/analysis/test_density.py::TestDensityAnalysis::test_run[dynamic]

8.56s call     MDAnalysisTests/coordinates/test_gro.py::TestGROLargeWriter::test_writer_large

8.44s call     MDAnalysisTests/analysis/test_gnm.py::test_closeContactGNMAnalysis

8.34s call     MDAnalysisTests/analysis/test_gnm.py::test_closeContactGNMAnalysis_weights_None

7.49s call     MDAnalysisTests/analysis/test_encore.py::TestEncore::test_ces_error_estimation_ensemble_bootstrap

richardjgowers · 2020-06-01T11:34:55Z

This looks cool. Would it make sense to put the new logic as a ResidueGroup method? (Psi_selections)

…

On Mon, Jun 1, 2020 at 12:21, Lily Wang ***@***.***> wrote: Fixes (part of) #2671 <#2671> Changes made in this Pull Request: - made phi/psi_selection longer, but faster with manual boolean array matching of atom names - made Ramachandran really long and ugly, but really faster with manual boolean array matching to atom names - no longer hardcodes phi/psi selection names in Ramachandran The original Ramachandran.__init__ with the GRO/XTC files was 180 s on my laptop. When I changed only the phi_selection code, it dropped to 122 s. When I changed the Ramachandran code, it dropped to 0.25 s. [image: Screenshot 2020-06-01 at 9 15 50 PM] <https://user-images.githubusercontent.com/31115101/83404225-9952d100-a44d-11ea-9d75-bc42ae972f76.png> How much testing does this need? PR Checklist - Tests? - Docs? - CHANGELOG updated? - Issue raised/referenced? ------------------------------ You can view, comment on, or merge this pull request online at: #2706 Commit Summary - select dihedrals faster File Changes - *M* package/MDAnalysis/analysis/dihedrals.py <https://github.com/MDAnalysis/mdanalysis/pull/2706/files#diff-ad86082633486f3093d80091122b3dc9> (111) - *M* package/MDAnalysis/core/topologyattrs.py <https://github.com/MDAnalysis/mdanalysis/pull/2706/files#diff-96c7f631815d065c6f087f4aea01a4e6> (123) Patch Links: - https://github.com/MDAnalysis/mdanalysis/pull/2706.patch - https://github.com/MDAnalysis/mdanalysis/pull/2706.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2706>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGB5YOEYUQGI7BVCVYSDRUOFMZANCNFSM4NPXRAXA> .

lilyminium · 2020-06-01T11:44:53Z

@richardjgowers that's a great idea, thanks! While we're looking at phi/psi_selection, though, and given the recent discussion on residx vs resids, should we be using segid and resid to select atoms? (I kept this logic in the manual matching for now). I can see the importance of resid when the protein is missing a chunk in the middle, but how should segid be treated? If multiple segments are called the same thing, are they supposed to be treated as the same segment?

richardjgowers · 2020-06-01T12:01:51Z

I’m not enough of a bio person to really answer. I do know that resindices make no promises on ordering, it’s just the unique number we give to each residue.

…

On Mon, Jun 1, 2020 at 12:45, Lily Wang ***@***.***> wrote: @richardjgowers <https://github.com/richardjgowers> that's a great idea, thanks! While we're looking at phi/psi_selection, though, and given the recent discussion on residx vs resids, *should* we be using segid and resid to select atoms? (I kept this logic in the manual matching for now). I can see the importance of resid when the protein is missing a chunk in the middle, but how should segid be treated? If multiple segments are called the same thing, are they supposed to be treated as the same segment? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2706 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACGSGBYYNWLB5RHBUMNZPKTRUOIEHANCNFSM4NPXRAXA> .

codecov · 2020-06-01T12:20:28Z

Codecov Report

Merging #2706 into develop will decrease coverage by 0.03%.
The diff coverage is 91.21%.

@@             Coverage Diff             @@
##           develop    #2706      +/-   ##
===========================================
- Coverage    91.19%   91.16%   -0.04%     
===========================================
  Files          174      174              
  Lines        23700    23881     +181     
  Branches      3099     3120      +21     
===========================================
+ Hits         21614    21771     +157     
- Misses        1472     1488      +16     
- Partials       614      622       +8

Impacted Files	Coverage Δ
package/MDAnalysis/core/topologyattrs.py	`95.37% <89.55%> (-2.12%)`	⬇️
package/MDAnalysis/analysis/dihedrals.py	`96.72% <100.00%> (+0.19%)`	⬆️

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 0911ff3...c1827e7. Read the comment docs.

lilyminium · 2020-06-01T16:01:35Z

Ramachandran.__init__ is now ~25 lines shorter but takes 0.32 s on my laptop :-( I'll let other people decide if it's worth the slowdown.

Edit: closer to 15 lines, actually.

richardjgowers · 2020-06-01T16:04:55Z

package/MDAnalysis/analysis/dihedrals.py

        """Parameters
        ----------
        atomgroup : AtomGroup or ResidueGroup
            atoms for residues for which :math:`\phi` and :math:`\psi` are
            calculated
+        c_name: str (optional)
+            name for the backbone C atom


specify defaults for these in the docstring

and maybe add a line showing how they can be used

I remember discussing this with @orbeckst and coming to the agreement that defaults are in the function signature anyway, so it's easier for docs not to diverge if they're not specified in the docstring. Happy to go either way though

package/MDAnalysis/analysis/dihedrals.py

richardjgowers · 2020-06-01T16:06:44Z

package/MDAnalysis/core/topologyattrs.py


        .. versionadded:: 1.0.0
        """
        u = residues[0].universe
-        nxres = rview = np.array([None]*len(residues), dtype=object)
+        nxres = np.array([None]*len(residues), dtype=object)


fwiw, seems like a candidate for atomgroupgroup #1861

richardjgowers

Looks good, I'll let someone else have a look before I merge it

richardjgowers

Ok test times look sub 40mins now, thanks @lilyminium !

lilyminium marked this pull request as draft June 1, 2020 12:05

richardjgowers self-assigned this Jun 1, 2020

lilyminium marked this pull request as ready for review June 1, 2020 16:01

richardjgowers reviewed Jun 1, 2020

View reviewed changes

package/MDAnalysis/analysis/dihedrals.py Outdated Show resolved Hide resolved

richardjgowers reviewed Jun 1, 2020

View reviewed changes

lilyminium added 4 commits June 2, 2020 02:31

select dihedrals faster

9b5d8d0

added residuegroup dihedrals and refactored residue dihedrals

56fa4e8

updated Rama and added function to get previous residues

fb29974

moved docstring to class, added example, updated CHANGELOG

c1827e7

lilyminium force-pushed the faster-dih branch from bb27a55 to c1827e7 Compare June 1, 2020 16:32

richardjgowers approved these changes Jun 3, 2020

View reviewed changes

richardjgowers merged commit 8c3577f into MDAnalysis:develop Jun 3, 2020

orbeckst mentioned this pull request Jun 4, 2020

performance benchmarks for dihedral analysis #2710

Open

lilyminium deleted the faster-dih branch June 6, 2020 11:59

orbeckst mentioned this pull request Jun 13, 2020

simple selection by name is slow due to fnmatch #2751

Closed

fiona-naughton added enhancement Component-Analysis labels Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select dihedrals faster #2706

Select dihedrals faster #2706

lilyminium commented Jun 1, 2020 •

edited

Loading

richardjgowers commented Jun 1, 2020 via email

lilyminium commented Jun 1, 2020

richardjgowers commented Jun 1, 2020 via email

codecov bot commented Jun 1, 2020 •

edited

Loading

lilyminium commented Jun 1, 2020 •

edited

Loading

richardjgowers Jun 1, 2020

richardjgowers Jun 1, 2020

lilyminium Jun 1, 2020

richardjgowers Jun 1, 2020

richardjgowers left a comment

richardjgowers left a comment

Select dihedrals faster #2706

Select dihedrals faster #2706

Conversation

lilyminium commented Jun 1, 2020 • edited Loading

PR Checklist

To do

richardjgowers commented Jun 1, 2020 via email

lilyminium commented Jun 1, 2020

richardjgowers commented Jun 1, 2020 via email

codecov bot commented Jun 1, 2020 • edited Loading

Codecov Report

lilyminium commented Jun 1, 2020 • edited Loading

richardjgowers Jun 1, 2020

Choose a reason for hiding this comment

richardjgowers Jun 1, 2020

Choose a reason for hiding this comment

lilyminium Jun 1, 2020

Choose a reason for hiding this comment

richardjgowers Jun 1, 2020

Choose a reason for hiding this comment

richardjgowers left a comment

Choose a reason for hiding this comment

richardjgowers left a comment

Choose a reason for hiding this comment

lilyminium commented Jun 1, 2020 •

edited

Loading

codecov bot commented Jun 1, 2020 •

edited

Loading

lilyminium commented Jun 1, 2020 •

edited

Loading