Expose computed density in InterRDF results by sumitsaraswat362 · Pull Request #5482 · MDAnalysis/mdanalysis

sumitsaraswat362 · 2026-10-10T10:04:15Z

Fixes #5362

Changes made in this Pull Request:

Expose computed average number density as results.density (and alias results.rho) in InterRDF and InterRDF_s when norm is "rdf" or "density".
Guard against zero-division / zero box volume cases when computing density.
Add unit tests for exposed density in test_rdf.py and test_rdf_s.py.
Update package/CHANGELOG and package/AUTHORS.

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)
I have read and understand the current AI Policy
LLM/AI disclosure was updated.

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

read-the-docs-community · 2026-10-10T10:08:54Z

Documentation build overview

📚 MDAnalysis | 🛠️ Build #35259985 | 📁 Comparing 436b64a against latest (4f6d54a)

🔍 Preview build

17 files changed · ± 17 modified

± Modified

codecov · 2026-10-10T10:24:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.92%. Comparing base (6cd1f7e) to head (436b64a).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #5482   +/-   ##
========================================
  Coverage    93.91%   93.92%           
========================================
  Files          184      184           
  Lines        22644    22661   +17     
  Branches      3219     3223    +4     
========================================
+ Hits         21267    21284   +17     
  Misses         917      917           
  Partials       460      460

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sici17 · 2026-10-11T16:18:21Z

I think that trying to compute the average density value in the norm="density" case might lead to zero division erros given that a nonzero box_vol is not actually required in these cases in order to compute the distance dependent density. Probably the two cases should be handled separately. I am talking about the line 401 of the diff. I am not an expert or anything though so I would wait someone with more expertise to express his opinion obviously. I was just practicing code reviewing and understing.

- Only compute results.density/results.rho when box_vol > 0, otherwise set to None - Expose results.rho alias alongside results.density as suggested in MDAnalysis#5362

sumitsaraswat362 · 2026-10-12T04:41:50Z

Thanks for the great catch, @sici17! You are completely right—for non-periodic trajectories or simulations without box dimensions where norm="density" is used, box_vol can be zero, which would have triggered an unintended ZeroDivisionError.

I've just pushed a commit guarding against this:

results.density (and results.rho) is now only computed when box_vol > 0, and set to None if no box volume is available.
In the norm="rdf" path, it proceeds with normalization as before.
Also exposed results.rho as an alias alongside results.density as originally suggested in Calculating the rdf for norm = "rdf" or "density" at the same time. #5362.

Really appreciate you taking the time to review!

orbeckst · 2026-10-13T20:20:43Z

Before we can review this PR, please restore the original PR template https://github.com/MDAnalysis/mdanalysis/blob/develop/.github/PULL_REQUEST_TEMPLATE.md and answer all questions.

…DF (MDAnalysis#5362)

sumitsaraswat362 · 2026-10-14T03:06:48Z

@orbeckst I have restored the official PR template in the PR description and answered all questions, updated package/CHANGELOG, added my name to package/AUTHORS, and added unit tests in test_rdf.py and test_rdf_s.py.

@sici17 Good catch! I added guards for zero box volume / non-periodic systems (box_vol > 0 and volume_cum > 0) so that results.density and results.rho are cleanly set to None without raising any ZeroDivisionError.

…DF_s

orbeckst

Quick initial review

only include results.density (drop results.rho) — there should be only one way to access the data
add documentation, for instance, results.density needs to be explained where the other results attributes are listed; add a .. versionadded:: 2.11.0

These are the first things I noticed, there may be more comments.

orbeckst · 2026-10-16T07:49:15Z

Please also resolve the conflicts with the develop branch.

sumitsaraswat362 · 2026-10-16T09:24:29Z

@orbeckst Thanks for the review!

I have addressed the points:

Dropped results.rho completely so results.density is the sole way to access the data.
Added documentation for results.density under the Attributes section of both InterRDF and InterRDF_s with .. versionadded:: 2.11.0.
Merged the latest develop branch and resolved the merge conflict in package/CHANGELOG.
Updated test assertions in test_rdf.py and test_rdf_s.py.

orbeckst

The computed "density" is $N_a N_b / \langle V \rangle$. This is not a real density and it's not the $\rho$ from the docs at the top. One could write it as $N_a \rho_b = N_b \rho_a$. We cannot store this as an attribute results.density because it's not a density.

(Furthermore, it's confusing because norm="rdf gives results.rdf and then people/LLMs may look for results.density for norm="density".)

We could store it as results.normalization_density or something like that. What would be a good name? Does it still make sense to store this quantity or should we be storing $\rho_a$, $\rho_b$, $\langle V \rangle$ as well?

Whatever we store, we need then document properly, i.e., write down the equations that define the quantity.

We cannot really continue with the PR until we have decided this.

…ties (MDAnalysis#5362)

sumitsaraswat362 · 2026-10-17T18:51:10Z

Thank you for pointing this out, Oliver. You are completely right. The quantity $N_a N_b / \langle V \rangle$ is the pair normalization density $N_a \rho_b$ rather than the single-species density $\rho$, and exposing it as results.density is inaccurate and misleading for coordination calculations ($N_{ab} = \rho G_{ab}$). It also collides with norm="density" where users expect the radial profile.

I have updated the implementation to address this:

results.normalization_density: Renamed from results.density, storing the pair normalization density $N_{\text{pairs}} / \langle V \rangle = (N_a N_b - N_{\text{excl}}) / \langle V \rangle$ used in the RDF denominator (or $1 / \langle V \rangle$ for InterRDF_s).
results.box_volume: Stores the trajectory-averaged box volume $\langle V \rangle$ across analyzed frames.
results.density_a and results.density_b: Store the true bulk number densities $\rho_a = N_a / \langle V \rangle$ and $\rho_b = N_b / \langle V \rangle$, where $\rho_b$ is the density $\rho$ appearing in the documentation for $n_{ab}(r) = \rho g_{ab}(r)$ and $N_{ab}(r) = \rho G_{ab}(r)$.
If the box volume is zero or unavailable, all four attributes evaluate to None.
When norm="none", none of these normalization attributes are set.

The equations and physical definitions have been added to the docstrings of both InterRDF and InterRDF_s with .. versionadded:: 2.11.0, and the test suites in test_rdf.py and test_rdf_s.py have been updated with numerical assertions against the theoretical values.

orbeckst

I like your new attributes and docs. Please see comment below for necessary updates.

orbeckst · 2026-10-19T19:17:30Z

+    s1, s2 = sels
+    # When norm is "rdf"
+    rdf = InterRDF(s1, s2, norm="rdf").run(**client_InterRDF)
+    assert not hasattr(rdf.results, "density")


Remove this assertion — why is this here??

orbeckst · 2026-10-19T19:17:40Z

+
+    # When norm is "density"
+    rdf_dense = InterRDF(s1, s2, norm="density").run(**client_InterRDF)
+    assert not hasattr(rdf_dense.results, "density")


Remove this assertion — why is this here??

orbeckst · 2026-10-19T19:22:43Z

+       .. versionadded:: 2.11.0
+
+    results.density_a : float
+       The bulk number density :math:`\rho_a = N_a / \langle V \rangle` of the


rename to density_1 and density_2 and change the equations in the doc string to use 1 instead of a and 2 instead of b.

In this way it better matches g1 and g2.

I think we can leave the general docs at the module level. Readers should be able to make the jump between a=1 and b=2.

orbeckst · 2026-10-19T19:23:01Z

+
+       .. versionadded:: 2.11.0
+
+    results.density_b : float


orbeckst · 2026-10-19T19:23:34Z

           This attribute will be removed in 3.0.0.
           Use :attr:`results.count` instead.

+    results.normalization_density : float


use a=1 and b=2

orbeckst · 2026-10-19T19:30:26Z

+    s1 = u.select_atoms("name OW")
+    s2 = u.select_atoms("name HW1 HW2")
+    rdf = InterRDF(s1, s2, norm="density").run(**client_InterRDF)
+    assert not hasattr(rdf.results, "density")


??? Is this an LLM remnant ???

orbeckst · 2026-10-19T19:30:51Z

    kwargs = {"density": density} if density is not None else {}
    rdf = InterRDF_s(u, sels, **kwargs).run(**client_InterRDF_s)
    assert_almost_equal(max(rdf.results.rdf[0][0][0]), value)
+    assert not hasattr(rdf.results, "density")


orbeckst · 2026-10-19T19:31:11Z

+    assert not hasattr(rdf.results, "density")
+    assert hasattr(rdf.results, "box_volume")
+    assert hasattr(rdf.results, "normalization_density")
+    assert rdf.results.box_volume > 0


assert a value

orbeckst · 2026-10-19T19:32:21Z

+    s2 = u.select_atoms("name OD1 and resid 51")
+    ags = [[s1, s2]]
+    rdf = InterRDF_s(u, ags, norm="density").run(**client_InterRDF_s)
+    assert not hasattr(rdf.results, "density")


orbeckst · 2026-10-19T19:32:48Z

 Enhancements
+ * InterRDF and InterRDF_s now expose computed average box volume and densities
+   as results.box_volume, results.normalization_density, and (for InterRDF)
+   results.density_a and results.density_b when norm is "rdf" or "density"


rename, see comments

…MDAnalysis#5362)

sumitsaraswat362 · 2026-10-20T04:17:12Z

Thanks for the careful review, @orbeckst! I have addressed all feedback in the latest commits:

Attribute & Doc Renaming:
- Renamed density_a and density_b to density_1 and density_2 across InterRDF, docstrings, and CHANGELOG.
- Updated docstring formulas to use indices 1 and 2 ($N_{\text{pairs}} = N_1 N_2 - N_{\text{excl}}$, $\rho_1 = N_1 / \langle V \rangle$, $\rho_2 = N_2 / \langle V \rangle$) to match g1 and g2.
Cleaned up tests:
- Good catch on the assert not hasattr(..., "density") checks — these were leftover artifacts from early drafts of the PR and have been completely removed.
- In test_rdf.py, parametrized test_exposed_density over norm in ("rdf", "density") and separated norm="none" into its own test function.
- Replaced assert box_volume > 0 with exact reference values: $10^6,\text{Å}^3$ for the $100,\text{Å}$ box in test_rdf.py, and the mean trajectory volume $\frac{1}{N}\sum V_i$ in test_rdf_s.py.
- Verified formatting with black (line-length = 79).

Expose computed density in InterRDF results (MDAnalysis#5362)

89fca38

Sumit Saraswat added 2 commits October 10, 2026 15:42

Fix black formatting

4d8834b

Fix volume_cum initialization in _prepare for norm=density

6eb8e76

Guard against ZeroDivisionError when box_vol is zero in norm='density'

d6cd1fd

- Only compute results.density/results.rho when box_vol > 0, otherwise set to None - Expose results.rho alias alongside results.density as suggested in MDAnalysis#5362

orbeckst added the more information needed Please reply to requests for information or the issue will be closed. label Oct 13, 2026

Add tests, update CHANGELOG and AUTHORS for exposed density in InterR…

7b16b8e

…DF (MDAnalysis#5362)

Add tests for zero box volume density fallback in InterRDF and InterR…

9e8f74a

…DF_s

orbeckst removed the more information needed Please reply to requests for information or the issue will be closed. label Oct 15, 2026

Merge branch 'develop' into fix-rdf-density-dev

e5573e9

orbeckst requested changes Oct 16, 2026

View reviewed changes

Sumit Saraswat added 2 commits October 16, 2026 14:46

Merge branch 'origin/develop' into fix-rdf-density-dev

07397e3

DOC, ENH: document results.density and drop results.rho

deb1c8a

MAINT: fix trailing newline in test_rdf.py for black

359495f

orbeckst requested changes Oct 17, 2026

View reviewed changes

DOC, ENH: store normalization_density, box_volume and selection densi…

8e84db0

…ties (MDAnalysis#5362)

Sumit Saraswat and others added 2 commits October 18, 2026 00:24

MAINT: format test files with black

47aac89

Merge branch 'develop' into fix-rdf-density-dev

34ab5bd

orbeckst requested changes Oct 19, 2026

View reviewed changes

Sumit Saraswat added 2 commits October 20, 2026 09:32

DOC, TEST, MAINT: rename to density_1/density_2 and parametrize tests (…

a3c97e0

…MDAnalysis#5362)

MAINT: format with black (line-length 79)

436b64a

Conversation

sumitsaraswat362 commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LLM / AI generated code disclosure

PR Checklist

Developers Certificate of Origin

Uh oh!

read-the-docs-community Bot commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

codecov Bot commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sici17 commented Oct 11, 2026

Uh oh!

sumitsaraswat362 commented Oct 12, 2026

Uh oh!

orbeckst commented Oct 13, 2026

Uh oh!

sumitsaraswat362 commented Oct 14, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Oct 16, 2026

Uh oh!

sumitsaraswat362 commented Oct 16, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

sumitsaraswat362 commented Oct 17, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sumitsaraswat362 commented Oct 20, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sumitsaraswat362 commented Oct 10, 2026 •

edited

Loading

read-the-docs-community Bot commented Oct 10, 2026 •

edited

Loading

codecov Bot commented Oct 10, 2026 •

edited

Loading