Skip to content

Added a norm attribute to InterRDF classes #3687

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

Merged
merged 6 commits into from
Jun 7, 2022

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented May 31, 2022

Changes made in this Pull Request:

This PR adds a norm attribute to the two RDF classes InterRDF and InterRDF_s. It works similarly to the density parameter which was only available for InterRDF_s.

Also, the universe attribute in InterRDF_s is deprecated because the universe is directly available via the AtomGroups.

Finally, I slightly cleaned up the code and moved the attributes within the classes themselves.

PR Checklist

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

This PR adds a `norm` attribute to the two rdf classes
`InterRDF` and `InterRDF_s`. It works similar as the density
parameter which was only available for `InterRDF_s`.

Also, the universe attribute in `InterRDF_s` is
depricated because the universe can be accessed via
the AtomGroups.

Finally, I slightly cleaned up the code and moved the
attributes within the classes themselves.
@pep8speaks
Copy link

pep8speaks commented May 31, 2022

Hello @PicoCentauri! 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 2022-06-07 07:26:38 UTC

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #3687 (53b05da) into develop (d0be0e6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3687   +/-   ##
========================================
  Coverage    94.35%   94.35%           
========================================
  Files          191      191           
  Lines        24975    24984    +9     
  Branches      3365     3375   +10     
========================================
+ Hits         23565    23574    +9     
  Misses        1362     1362           
  Partials        48       48           
Impacted Files Coverage Δ
package/MDAnalysis/analysis/rdf.py 100.00% <100.00%> (ø)

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 d0be0e6...53b05da. Read the comment docs.

@PicoCentauri PicoCentauri force-pushed the rdf_norm branch 2 times, most recently from f7a6009 to 95b5027 Compare May 31, 2022 18:47
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.

LGTM

@orbeckst orbeckst added this to the 2.3.0 milestone Jun 2, 2022
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

please update CHANGELOG --- new features should show up

@orbeckst orbeckst self-assigned this Jun 2, 2022
@PicoCentauri
Copy link
Contributor Author

I bumped the version informations to 2.3.0 and added a note to the CHANGELOG. Thanks at @orbeckst and @richardjgowers for rewieing.

@IAlibay
Copy link
Member

IAlibay commented Jun 3, 2022

I bumped the version informations to 2.3.0 and added a note to the CHANGELOG. Thanks at @orbeckst and @richardjgowers for rewieing.

Did you push? The changes aren't showing up here for some reason.

@PicoCentauri
Copy link
Contributor Author

Did you push? The changes aren't showing up here for some reason.

of course not 😅

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

LGTM

(Minor formatting comment but that's all. Thanks for the clean-up!)

The `exclusion_block` keyword allows the masking of pairs from
within the same molecule. For example, if there are 7 of each
atom in each molecule, the exclusion mask `(7, 7)` can be used.
The ``exclusion_block`` keyword allows the masking of pairs from
Copy link
Member

@orbeckst orbeckst Jun 3, 2022

Choose a reason for hiding this comment

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

We actually use the single backticks to refer to keyword arguments (which uses just the default reST role), although this may be inconsistent throughout.

Copy link
Member

Choose a reason for hiding this comment

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

😅 o we should have docs about that because I've definitely been using double backticks for keywords because otherwise the italics is hard to read (and I use the :attr: and :meth: for other things)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. I changed the docs and it actually also looks cleaner when one is able to distinguish between arguments and inline code snippets.

@orbeckst orbeckst merged commit 56cbf56 into MDAnalysis:develop Jun 7, 2022
@PicoCentauri PicoCentauri deleted the rdf_norm branch June 5, 2024 15:55
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.

5 participants