Skip to content

ENH:Add 3D PDF calculator support#49

Open
joeseaer wants to merge 6 commits into
diffpy:3DPDFfrom
joeseaer:main
Open

ENH:Add 3D PDF calculator support#49
joeseaer wants to merge 6 commits into
diffpy:3DPDFfrom
joeseaer:main

Conversation

@joeseaer

@joeseaer joeseaer commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Hi @sbillinge @dragonyanglong ,

This PR adds a CPU PDF3DCalculator implementation to libdiffpy. The calculator
produces 3D real-space PDF data on a cubic grid, exposes radial histogram data,
supports binary dense-grid export, q-window post-processing, rho0 background
scaling, and 3D covariance delta controls.

It also includes the small supporting changes needed by the calculator:
a symmetric 3x3 eigen-solver helper in R3linalg, a PDFCalculator helper for
partial PDF scale, and an ObjCryst translation-vector type compatibility fix
for builds where ObjCryst::REAL is double.

I tested the libdiffpy build locally with ObjCryst/GSL/Boost available.

@sbillinge sbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't see any tests. Are there tests?

Can we also try and think about if it makes sense to make shared functions for anything to avoid copy-paying code from the PDF calculator? The more we can separate into shared functions the less duplication we have to maintain in the future

Comment thread src/diffpy/srreal/PDF3DCalculator.hpp Outdated
*
* libdiffpy by DANSE Diffraction group
* Simon J. L. Billinge
* (c) 2009 The Trustees of Columbia University

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please fix date and address. I am not sure we actually need this text block any more as we do the copyright and attribution differently do maybe just delete the whole block?

@joeseaer

joeseaer commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks Simon. I updated the PR to address these review comments.

I removed the copied legacy copyright/header blocks from the new
PDF3DCalculator.cpp and PDF3DCalculator.hpp files.

I also refactored the shared structure-factor and normalization logic so
PDF3DCalculator now reuses PDFCalculator helpers for site scattering factors,
partial PDF scale, and RDF scale instead of maintaining its own duplicate cache.
The duplicated 3D post-processing path used by get3DPDF and exportGrid3DBinary
has also been consolidated.

The 3D grid deposition, spherical q-window, and delta-shell preprocessing remain
inside PDF3DCalculator because they operate on the 3D grid representation and do
not map directly to the existing 1D PDFCalculator routines.

I verified the updated libdiffpy build locally with scons -j2.

@sbillinge

Copy link
Copy Markdown
Contributor

This is great. Thanks. Please can you paste the results of running tests locally?

Also, possibly on a new branch, please could you try and merge migration-nano into this branch and see if the tests still pass.. Actually, I would rather merge your changes into that branch so that it is coded at the version of the next release

@dragonyanglong

Copy link
Copy Markdown
Contributor

@joeseaer any updates on this?

@joeseaer

Copy link
Copy Markdown
Contributor Author

Sorry for the delayed update.
I have completed the naming changes from PDF3DCalculator to ThreeDPDFCalculator, including the file names, class and constructor, includes, tests, result accessor, and serialization registration. I also verified that the old name no longer appears in the code or file names.
I expanded the C++ test coverage and ran the full test suite locally:

python3 -m SCons test

Running cxxtest tests (218 tests) ... OK!
scons: done building targets.
TEST EXIT CODE: 0

This was tested on WSL2 Ubuntu 24.04 with GCC 13.3.0, Python 3.12.3, Boost 1.83.0, GSL 2.7.1, and libobjcryst 2022.1.4.

I am now reviewing the work on migration-nano and confirming the integration details and required code changes with Rundong. Once that work is complete, I will push the integrated changes to a branch based on migration-nano and update this PR to target that branch.

@dragonyanglong

Copy link
Copy Markdown
Contributor

Hi Simon @sbillinge , renaming work is finished. After you or Rundong create a migration branch, qiaohai will make a PR to that specific branch.

@dragonyanglong

Copy link
Copy Markdown
Contributor

If the plan sounds good to you, please close this PR, no merging needed. We will make a new PR to the new migration branch.

@sbillinge

Copy link
Copy Markdown
Contributor

Hi Simon @sbillinge , renaming work is finished. After you or Rundong create a migration branch, qiaohai will make a PR to that specific branch.

I am a little unclear on the request here. migration-nano branch exists and the nanobind work is over there. @joeseaer reference this branch so he knows about it. Please could you clarify what you need?

@sbillinge

Copy link
Copy Markdown
Contributor

If the plan sounds good to you, please close this PR, no merging needed. We will make a new PR to the new migration branch.

I can update the base branch of this PR so you may not need to redo the PR, though you may want to is you build a new branch from that new base. Just let me know what you decide. I will leave it open for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants