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

Streamline angles(), bonds(), dihedrals() #3203

Merged
merged 2 commits into from
May 2, 2021

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Apr 3, 2021

Fixes #

Changes made in this Pull Request:

  • Wrote an underlying function for angles(), bonds(), dihedrals() to use
  • This doesn't really change the code. It adds a little execution time in calling another function, creating a new list of positions, and a bit of string processing for the error messages. It removes a little maintenance time because now any changes only have to be made to one function.

Is there a reason for the _bondsSlow(), _anglesSlow(), etc. functions? They don't appear to be used anywhere.

PR Checklist

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

@pep8speaks
Copy link

pep8speaks commented Apr 3, 2021

Hello @lilyminium! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-23 04:25:53 UTC

@codecov
Copy link

codecov bot commented Apr 3, 2021

Codecov Report

Merging #3203 (cc9aeae) into develop (bb5d291) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3203      +/-   ##
===========================================
+ Coverage    92.73%   92.80%   +0.07%     
===========================================
  Files          168      170       +2     
  Lines        22686    22792     +106     
  Branches      3214     3233      +19     
===========================================
+ Hits         21038    21153     +115     
+ Misses        1600     1591       -9     
  Partials        48       48              
Impacted Files Coverage Δ
package/MDAnalysis/core/topologyobjects.py 98.56% <100.00%> (-0.05%) ⬇️
package/MDAnalysis/analysis/rms.py 95.06% <0.00%> (ø)
package/MDAnalysis/transformations/fit.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/wrap.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/rotate.py 100.00% <0.00%> (ø)
package/MDAnalysis/transformations/__init__.py 100.00% <0.00%> (ø)
...ge/MDAnalysis/transformations/positionaveraging.py 100.00% <0.00%> (ø)
...sis/analysis/encore/clustering/ClusteringMethod.py 96.92% <0.00%> (ø)
package/MDAnalysis/transformations/base.py 100.00% <0.00%> (ø)
...ackage/MDAnalysis/transformations/boxdimensions.py 100.00% <0.00%> (ø)
... and 11 more

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 bb5d291...cc9aeae. Read the comment docs.

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

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

The slow versions were for debugging back in the day, if you want to remove them then that would be good

@IAlibay IAlibay added this to the 2.0 milestone Apr 6, 2021
@IAlibay IAlibay requested a review from mnmelo April 22, 2021 18:02
@richardjgowers richardjgowers merged commit fe22dc3 into MDAnalysis:develop May 2, 2021
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