Skip to content

Add finite cluster library generation and PDF screening via MCP - #56

Open
sbanerjee23 wants to merge 1 commit into
diffpy:mainfrom
sbanerjee23:feat/cluster-mining-pr
Open

sbanerjee23 wants to merge 1 commit into
diffpy:mainfrom
sbanerjee23:feat/cluster-mining-pr

Conversation

@sbanerjee23

Copy link
Copy Markdown

What problem does this PR address?

This adds an in-memory workflow for PDF screening across multiple candidate structural models, implemented here for finite metallic clusters. It coordinates refinements under consistent conditions and compares their results using the existing RefinementSession infrastructure.

This PR adds two reusable operations:

  • generate_cluster_library — Generates bounded libraries of finite metallic clusters using ASE. Supported structures include icosahedral (Mackay/magic), FCC (octahedral/truncated/cuboctahedral), and Decahedral (regular/Ino/Marks/Ino-truncated Marks) families. Candidate structures are generated in memory from explicit specifications or parameter grids.
  • screen_cluster_library — Refines, compares, and ranks candidate structures against an experimental PDF. Each candidate is refined using the existing RefinementSession infrastructure, with consistent fitting parameters. The results include Rw, refined parameters, convergence status, and execution times.

Both operations are exposed as MCP tools through refinement_server.py. ASE is added as a dependency.

The default screening recipe refines scale, isotropic structural scaling, shared Uiso, and delta2, with optional parameterization and constraint extensions. Cluster refinements are performed sequentially, with optional CPU parallelization within individual PDF calculations.

Tests: Added coverage for cluster generation, screening, MCP integration, and a numerical regression test using experimental Pd PDF data.

Scope: Candidate-selection strategies, computational-budget estimation, and screening-map visualization are handled at the agent-workflow level. A companion cmi-agent-skills PR will provide guidance.

What should the reviewer(s) do?

Please review the cluster-generation and screening interfaces, their integration with the existing MCP server, and the accompanying tests. Feedback on the separation between the screening workflow and the general-purpose refinement infrastructure would be helpful.

  • This PR introduces a public-facing change.
    • Documentation has been updated.
    • Documentation plan: companion agent skill PR and follow-up usage documentation.

Add ASE-based generation of bounded metallic-cluster libraries and
a screening workflow that refines, compares, and ranks candidate
structures from these libraries, reusing the existing RefinementSession.

Expose both operations as MCP tools and add generation, screening,
MCP, and experimental PDF regression tests.
@codecov

codecov Bot commented Nov 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.60784% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.66%. Comparing base (93fcf3c) to head (b22538c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_cluster_options.py 99.35% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   99.69%   99.66%   -0.03%     
==========================================
  Files           8       12       +4     
  Lines         648      903     +255     
==========================================
+ Hits          646      900     +254     
- Misses          2        3       +1     
Files with missing lines Coverage Δ
tests/test_cluster_generation.py 100.00% <100.00%> (ø)
tests/test_cluster_mcp.py 100.00% <100.00%> (ø)
tests/test_cluster_screening.py 100.00% <100.00%> (ø)
tests/test_cluster_options.py 99.35% <99.35%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbanerjee23

Copy link
Copy Markdown
Author

@ycexiao ready for review

@ycexiao

ycexiao commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Hi @sbanerjee23. Thanks for your contribution! The code overall is good to me.

My initial expectation of this library is to give granular tools to achieve flexible usage of diffpy.cmi without referring to the complicated code. But now I realized people would always like more tools and more accessible functionalities whenever they need it, and I think more adding tools like this one is always a great idea.

What makes me slightly hesitated to merge is that it seems to me some of steps in this PR can be done by "Agent", and the two function generate_cluster_library and screen_cluster_library hard-coded a little too many functionalities in only two interface. The idea is that Implementing minimal functionalities in the code and providing maximal interface enables maximal flexibility of the agent.

I’m happy to merge it if we can refactor them in the future. If you have time, though, it would be better to address this technical debt before it accumulates.

@sbillinge could you please check this PR?

@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.

before we merge this, let's think a bit about how to do it in a way that is extensible. @sbanerjee23 you are adding new functionality. I could imagine that many people will get excited and want to add functionality. Is there a way we can do this in a separable way that doesn't make the apps more and more complex? I am honestly not sure that it is worth it, but let's give it a few minutes thought and decide. We did this with diffpy.cmi so it now has packs that can be installed separately. It was suggested to us that we should have used sub-packages as an easier workaround. I wonder if a solution like that could work here. The "packs" is alot of extra infrastructure and probably not yet warranted, though the later we leave it, the more refactoring there would be.

@sbanerjee23

sbanerjee23 commented Nov 3, 2026 •

Copy link
Copy Markdown
Author

Hi guys @sbillinge @ycexiao, no problem at all to hold off on merging and think more carefully about what we're actually adding here.

The way I see it, the value of the MCP approach isn't necessarily that it enables analyses an agent couldn't otherwise perform. For example, an agent given the methods section from the ClusterMining paper could write its own DiffPy scripts, build a new Numba-accelerated fast PDF calculator from scratch, choose its own termination functions, or explore any number of different approaches. I've seen this, and some of those solutions can be very good, and other times they are not. I think there's value in deliberately constraining that flexibility to make the analysis more consistent, reproducible, and easier to validate. That is the feature itself. From an experimenter’s perspective, I sometimes want to reproduce a bespoke analysis, in a bespoke way, and other times, I am interested in a more exploratory approach.

The separation I imagined here is that much of the scientific decision-making and guidance lives in the accompanying CMI agent skill (diffpy/cmi-agent-skills#8), while the additions to diffpy.apps provide the minimal numerical operations needed to execute that workflow consistently. This PR leaves the existing parametric refinement infrastructure unchanged; the two new MCP tools call the screening modules, which use the existing RefinementSession engine and DiffPy PDF calculators for the actual refinements.

That said, the current division of functionality may not be the right one. The cluster generator is highly specific to three families of monometallic ASE structures, so its present implementation is necessarily tied to those particular builders. Perhaps it should be named more explicitly, something like generate_ase_monoatomic_cluster_lib(), and kept separate from diffpy.apps. We could also test whether an agent given the same CMI skill could handle the ASE structure generation itself and integrate that within the existing infrastructure, without needing a dedicated MCP tool. Naively, it seems like that could work smoothly, but I’m not sure without trying.

On the other hand, my broader goal for screening was indeed to move toward something more generalizable. I see a close connection between screening and mapping, although they aren't exactly the same thing (maybe more accurate to just call it the latter). Here we are sampling structural models of different sizes and evaluating their agreement with an experimental PDF, producing an Rw-versus-atom-count map. But conceptually, how different is that from sampling experimental positions (x, y) and mapping a corresponding quantity such as Rw, PCC, or max intensity? The underlying idea which might be good to generalize is: sample a structural or experimental parameter space, calculate a relevant metric at each point, and map its variation.

So, in writing this, it gives me quite a few ideas for benchmarks we can do, to really convince each other which parts add value and which do not. One thing is trying to move the structure generation out of diffpy.apps altogether and see if the quality of the workflow using the same (or slightly adjusted) CMI agent skill is unchanged. Another benchmark which should be high priority imo, would be to simply give “quarantined” agents the same scientific task and guidance (cmi-agent-skill), with and without the MCP server, and compare the consistency, reproducibility, and computational efficiency of the resulting analyses across a meaningful number of trial runs. If we can quantify the improvement in the analysis when using the MCP server (if that is true), I guess that would be convincing.

In any case, happy to refactor, and it might be good as a first step to decide on the benchmarks together. I also don’t know anything about the new cmi packs infrastructure but I will read up on it.

@sbillinge

Copy link
Copy Markdown
Contributor

@sbanerjee23 thanks for the thoughtful discussion. I think we are in pretty good agreement regarding the philosophy. Also, the scientific target and where "screening" could go.

My comments were more about the structure of the repo when it comes to "user" contributions. Some we want to incorporate into the base code, but some might be better as "extensions" that can be imported if and when a user wants to use them. I just wanted to think about, in general, how we might want to think about these things

This branch has not been deployed

No deployments
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.

3 participants