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

add ignore_same_residue kwarg to InterRDF #4161

Merged
merged 15 commits into from Jul 19, 2023

Conversation

orionarcher
Copy link
Contributor

@orionarcher orionarcher commented Jun 1, 2023

Changes made in this Pull Request:

This adds the ability to include the same residues in both g1 and g2 without blowing up the RDF calculation. Specifically, it adds a ignore_same_residue kwarg that excludes pairs of atoms with the same resindice from the distance calculation. This should be useful when a user wants to understand the correlation of a species with itself (as I do now).

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced? (N/A)

📚 Documentation preview 📚: https://mdanalysis--4161.org.readthedocs.build/en/4161/

@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Linter Bot Results:

Hi @orionarcher! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location Outcome
main package ⚠️ Possible failure
testsuite ⚠️ Possible failure

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/5591517434/jobs/10222703731


Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (2e39563) 93.61% compared to head (29606f2) 93.62%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4161      +/-   ##
===========================================
+ Coverage    93.61%   93.62%   +0.01%     
===========================================
  Files          193      193              
  Lines        25170    25295     +125     
  Branches      4059     4063       +4     
===========================================
+ Hits         23562    23683     +121     
- Misses        1092     1096       +4     
  Partials       516      516              
Impacted Files Coverage Δ
package/MDAnalysis/analysis/rdf.py 100.00% <100.00%> (ø)

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@orionarcher orionarcher marked this pull request as draft June 1, 2023 21:46
@orionarcher orionarcher marked this pull request as ready for review June 1, 2023 21:57
@orionarcher orionarcher changed the title add an ignore_same_residue kwarg to InterRDF and test it add ignore_same_residue kwarg to InterRDF Jun 1, 2023
@orionarcher orionarcher changed the title add ignore_same_residue kwarg to InterRDF add ignore_same_residue kwarg to InterRDF Jun 1, 2023
@orionarcher
Copy link
Contributor Author

Iterated slightly on this. Now users can select "residue", "segment", or "chain" to exclude. It will automatically map to the resindices, segindices, and chainID attributes (though I'm not sure chainid is the right option?). I feel this is a bit friendlier to new users than forcing them to understand the intricacies of 'residvsresindicesvsresname`, and covers the vast majority of use cases. Still happy to change it up if other implementations are preferred.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Quick comments.

package/MDAnalysis/analysis/rdf.py Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/rdf.py Outdated Show resolved Hide resolved
@orionarcher
Copy link
Contributor Author

Fixed @hmacdope's requested. Unless there are further requests for changes, this is ready to merge.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Few changes and a tiny bit more testing but almost there.

package/MDAnalysis/analysis/rdf.py Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/rdf.py Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/rdf.py Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/rdf.py Show resolved Hide resolved
testsuite/MDAnalysisTests/analysis/test_rdf.py Outdated Show resolved Hide resolved
package/CHANGELOG Outdated Show resolved Hide resolved
package/MDAnalysis/analysis/rdf.py Outdated Show resolved Hide resolved
testsuite/MDAnalysisTests/analysis/test_rdf.py Outdated Show resolved Hide resolved
@orbeckst
Copy link
Member

@hmacdope or @RMeli — could one of you assign the issue to yourself, please, just so that it's clear that someone is in charge following up on comments and eventually merging? Thanks.

@RMeli RMeli self-assigned this Jul 6, 2023
@orionarcher
Copy link
Contributor Author

I added new tests for segment and chain and added @RMeli cleanup suggestions. Thanks for all the feedback.

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

Thanks @orionarcher. A few last nitpicks (sorry), otherwise LGTM.

testsuite/MDAnalysisTests/analysis/test_rdf.py Outdated Show resolved Hide resolved
@RMeli
Copy link
Member

RMeli commented Jul 18, 2023

LGTM, thanks @orionarcher. I'll try to re-run the failed actions before approving, it failed because of time limits: https://dev.azure.com/mdanalysis/mdanalysis/_build/results?buildId=5981&view=logs&j=7a9c7d2c-346e-591b-84bb-5490fd358201

@RMeli
Copy link
Member

RMeli commented Jul 18, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@RMeli RMeli left a comment

Choose a reason for hiding this comment

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

Thanks @orionarcher. @hmacdope would you mind having the last look and merge? Thanks.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Looks great @orionarcher thanks so much. This is a great contribution and will be super useful to many.

@hmacdope hmacdope merged commit a1aff66 into MDAnalysis:develop Jul 19, 2023
24 checks passed
@orionarcher
Copy link
Contributor Author

Yay thanks y'all!

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

6 participants