Skip to content

refactor: adjust the internal interface and workflow for better agent performance - #48

Merged
sbillinge merged 6 commits into
diffpy:mainfrom
ycexiao:patch
Oct 17, 2026
Merged

sbillinge merged 6 commits into
diffpy:mainfrom
ycexiao:patch

Conversation

@ycexiao

@ycexiao ycexiao commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
  • refactor: refactor the code and fixed some small issues
  • refactor: rename the module refinebase as parametric_refinement

What problem does this PR address?

I asked Claude in the conversation where it manages to reproduce publication results this question "List the situations when diffpy.apps is not working as expected so I can fix".

It pumped 10 points. Three of these behaviors are expected, the rest is fixed by small refactoring in this PR:

1. get_model_residual overwrites whatever file data_path names.

What happened: I passed the profile’s own path and it silently replaced parent-uio66.gr with a list of residual values. The tool name sounds read-only, and the description doesn’t say it writes a file.
Fix: call the argument output_path, refuse to overwrite existing files unless told to, and refuse when the path matches a loaded profile.

2. finite defaults to False, so .xyz clusters are silently treated as periodic.

What happened: an .xyz file has lattice 1 Å, so the periodic calculator gives garbage: oscillating G(r) and Rw of 6–12 at the correct parameters. Nothing warns about it. With finite=True the same parameters give Rw 0.2742.
Fix: use the finite (Debye) calculator by default for .xyz input, or warn when the lattice is 1 × 1 × 1.

3. There’s no way to evaluate the model without refining.

What happened: solve with an empty variable list fails with “Variables or tags cannot be found (all)”. My workaround was to “refine” delta1 with lb = ub = 0.
Fix: add an evaluate/residual call that returns Rw and χ² at the current values, or let variable_names=[] do that.

4. get_model_evaluation crashes before any solve.

Error: TypeError: Profile.set_observed_profile() got an unexpected keyword argument 'xname'.
When: it works only after a solve has built a recipe.

5. The solver barely moves from the starting values.

What happened: in all 25 grid points, s2 changed by less than 0.005 and s1 by about 0.015.
Likely cause: it stops once a parameter reaches a restrained bound. g22 Zr3 Uiso hit 1e-4 with σ = 1e-7 every time.
Comparison: your least_squares(x_scale="jac") gets much further from generic starts (Rw ≈ 0.285, versus 0.43–0.48 here).
Fix: expose the optimiser settings (method, x_scale, max_nfev, ftol/xtol) and report why it stopped and how many function evaluations it used.

6. Long solves time out with no progress feedback.

What happened: the slow periodic solves took more than 60 s and the call timed out on my side. I then had to poll list_recipe_parameters to see whether it had finished.
Fix: run the solve as a background job that returns an ID, plus a status call.

8. Constraints carry over between solves in the same session.

Workaround: clear and rebuild everything before each fit.

What should the reviewer(s) do?

@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.68%. Comparing base (a517353) to head (020b736).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   98.24%   99.68%   +1.44%     
==========================================
  Files           8        8              
  Lines         626      634       +8     
==========================================
+ Hits          615      632      +17     
+ Misses         11        2       -9     
Files with missing lines Coverage Δ
tests/conftest.py 100.00% <100.00%> (+30.00%) ⬆️
tests/test_agentify.py 100.00% <100.00%> (ø)
tests/test_parametric_model.py 100.00% <100.00%> (ø)
tests/test_refinement_server.py 100.00% <100.00%> (ø)
tests/test_refinement_session.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ycexiao
ycexiao marked this pull request as ready for review October 15, 2026 20:11
@ycexiao

ycexiao commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

@sbillinge, it's ready for review.

@ycexiao
ycexiao marked this pull request as draft October 16, 2026 00:34
@ycexiao

ycexiao commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

converted to draft as new issues founded.

@ycexiao ycexiao closed this Oct 16, 2026
@ycexiao
ycexiao deleted the patch branch October 16, 2026 02:42
@ycexiao
ycexiao restored the patch branch October 16, 2026 02:42
@ycexiao ycexiao reopened this Oct 16, 2026
@ycexiao

ycexiao commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

tests_on_pr is failed because it depends on this PR in cmi-agent-skills repo: diffpy/cmi-agent-skills#4

@ycexiao
ycexiao marked this pull request as ready for review October 16, 2026 04:22
@ycexiao

ycexiao commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

@sbillinge, the tests_on_pr is fixed. It's ready for review.

@sbillinge
sbillinge merged commit 8145ae9 into diffpy:main Oct 17, 2026
6 checks passed
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.

2 participants