feat: Initialize FitRecipe with a results file or object by cadenmyers13 · Pull Request #166 · diffpy/diffpy.srfit

cadenmyers13 · 2026-02-26T20:29:48Z

No description provided.

cadenmyers13 · 2026-02-26T20:29:55Z

@sbillinge ready for review

cadenmyers13 · 2026-02-26T20:30:24Z

src/diffpy/srfit/fitbase/fitrecipe.py

+        if hasattr(results, "print_results"):
+            params_dict = utils.get_dict_from_results_object(results)
+        elif isinstance(results, (str, Path)):
+            params_dict = utils.get_dict_from_results_file(results)


handles either a results object or a path to a results file

cadenmyers13 · 2026-02-26T20:30:50Z

src/diffpy/srfit/fitbase/fitrecipe.py

+                param.name: param.getValue()
+                for param in self._parameters.values()
+            }
+            self._pretty_print_results_dict(set_parameters_dict)


Prints the parameters found in the results and the parameters set

cadenmyers13 · 2026-02-26T20:32:14Z

tests/conftest.py

+        recipe.add_variable(contribution.wave_number, 3)
+        recipe.add_variable(contribution.phase_shift, 2)
+        return recipe
+


had to wrap this in a second function because I realized calling the same fixture twice after the first one was refined led to initial values in the second call being the values of the previously refined recipe

I think you can simply change the scope of the fixture to remove this behavior

@sbillinge Unfortunately that doesn't work here. It was already set to scope="function" which is the lowest level so to speak.

ok, but I am confused. in this case it would reset every time through a pytest.mark.parametrize Are we not initializing something correctly? I am only banging on about this because it maybe suggests something may be wrong with our tests which would not be good.

@sbillinge In the current version, the test fixture is not being called twice when you do,

recipe1 = build_recipe_one_contribution recipe2 = build_recipe_one_contribution

What is happening here is that it is assigning the same fixture value to two variable names so recipe1 == recipe2 would return True even if one was refined and the other wasnt.

When we wrap it in another function like in the incoming version, each recipe object can be created

This may be indicating a weakness with our code, either the test or the code itself. Is it fixed if you instantiate recipe1 and recipe2 both at the top of the testing function?

@sbillinge I tested this without the conftest fixture and built the recipes manually like so,

def test_initialize_recipe_from_results_object(): # Case: User initializes a FitRecipe from a FitResults object # expected: recipe is initialized with variables from previous fit profile1 = Profile() x = linspace(0, pi, 10) y = sin(x) profile1.set_observed_profile(x, y) contribution1 = FitContribution("c1") contribution1.set_profile(profile1) contribution1.set_equation("amplitude*sin(wave_number*x + phase_shift)") recipe1 = FitRecipe() recipe1.add_contribution(contribution1) recipe1.add_variable(contribution1.amplitude, 4) recipe1.add_variable(contribution1.wave_number, 3) recipe1.add_variable(contribution1.phase_shift, 2) optimize_recipe(recipe1) results1 = FitResults(recipe1) expected_values = np.round(results1.varvals, 5) expected_names = results1.varnames profile2 = Profile() x = linspace(0, pi, 10) y = sin(x) profile2.set_observed_profile(x, y) contribution2 = FitContribution("c2") contribution2.set_profile(profile2) contribution2.set_equation("amplitude*sin(wave_number*x + phase_shift)") recipe2 = FitRecipe() recipe2.add_contribution(contribution2) recipe2.add_variable(contribution2.amplitude, 4) recipe2.add_variable(contribution2.wave_number, 3) recipe2.add_variable(contribution2.phase_shift, 2) recipe2.create_new_variable( "extra_var", 5 ) # should be included in the initialized recipe actual_values_before_init = [val for val in recipe2.get_values()] actual_names_before_init = recipe2.get_names() expected_names_before_init = [ "amplitude", "extra_var", "phase_shift", "wave_number", ] expected_values_before_init = [ 4, 3, 2, 5, ] # the three variables + the extra_var assert actual_values_before_init == expected_values_before_init assert sorted(actual_names_before_init) == sorted( expected_names_before_init ) recipe2.initialize_recipe_with_results(results1) optimize_recipe(recipe2) results2 = FitResults(recipe2) actual_values = np.round(results2.varvals, 5) actual_names = results2.varnames expected_names = expected_names + [ "extra_var" ] # add the new variable name to expected names expected_values = list(expected_values) + [ 5 ] # add the value of the new variable to expected values assert sorted(expected_names) == sorted(actual_names) assert sorted(expected_values) == sorted(list(actual_values))

Doing this passes the test meaning its a fixture related thing and not a code related thing, what we could do is have it like this for this specific test (and i think one other) and revert the conftest fixture back to original.

Is it fixed if you instantiate recipe1 and recipe2 both at the top of the testing function?

@sbillinge and no, this doesnt fix it

sbillinge

This looks very good. Please see a couple of comments

sbillinge · 2026-02-26T22:33:02Z

src/diffpy/srfit/fitbase/fitresults.py

+        The recipe from which the results were generated.
+
+    cov : numpy.ndarray or None
+        Covariance matrix of the refined variables. None if unavailable.


We seen to have lost a bunch of "The"s

sbillinge · 2026-02-26T22:34:19Z

tests/conftest.py

+        recipe.add_variable(contribution.wave_number, 3)
+        recipe.add_variable(contribution.phase_shift, 2)
+        return recipe
+


I think you can simply change the scope of the fixture to remove this behavior

cadenmyers13 · 2026-02-26T23:00:23Z

@sbillinge ready for review

codecov · 2026-02-27T01:38:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.81%. Comparing base (cee67de) to head (08478d4).
⚠️ Report is 20 commits behind head on v3.3.0.

Additional details and impacted files

@@            Coverage Diff             @@
##           v3.3.0     #166      +/-   ##
==========================================
+ Coverage   72.36%   72.81%   +0.45%     
==========================================
  Files          25       25              
  Lines        3832     3896      +64     
==========================================
+ Hits         2773     2837      +64     
  Misses       1059     1059

Files with missing lines	Coverage Δ
tests/conftest.py	`92.40% <100.00%> (+0.51%)`	⬆️
tests/test_fitrecipe.py	`99.85% <100.00%> (+0.01%)`	⬆️
tests/test_fitresults.py	`99.35% <100.00%> (ø)`

🚀 New features to boost your workflow:

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

cadenmyers13 · 2026-02-27T20:08:29Z

@sbillinge ready for revie

sbillinge

One comment. I am still a bit concerned with our testing workaround that I am worried is indicating a weakness in either our code or our test. I commented on that.

Also, I am not sure whether my other comment to always use an odd number for npoints in linspace actually went through, so let me say it here.....

tests/conftest.py

sbillinge · 2026-02-27T20:34:44Z

tests/conftest.py

+        recipe.add_variable(contribution.wave_number, 3)
+        recipe.add_variable(contribution.phase_shift, 2)
+        return recipe
+


This may be indicating a weakness with our code, either the test or the code itself. Is it fixed if you instantiate recipe1 and recipe2 both at the top of the testing function?

cadenmyers13 · 2026-02-27T20:51:58Z

@sbillinge Ready for review

One comment. I am still a bit concerned with our testing workaround that I am worried is indicating a weakness in either our code or our test. I commented on that.
Also, I am not sure whether my other comment to always use an odd number for npoints in linspace actually went through, so let me say it here.....

It didn't but now i see it and got it fixed 👍

sbillinge · 2026-02-27T22:53:39Z

I am having a hard time understanding this. Let's maybe jump on a call when we can and discuss it?

cadenmyers13 · 2026-03-02T14:48:29Z

@sbillinge sure thing. we can talk about it during our one-on-one. In general though, the pytest fixture hands out an already created FitRecipe object. It does not generate one, so it exists in the same place in memory. Calling the fixture again is not generating a new fit recipe object.

For example, in the current version doing,

assert recipe1 != recipe2 # check to see if they are identical

fails like so,

FAILED tests/test_fitrecipe.py::test_initialize_recipe_from_results_object - assert <diffpy.srfit.fitbase.fitrecipe.FitRecipe object at 0x1807d8aa0> != <diffpy.srfit.fitbase.fitrecipe.FitRecipe object at 0x1807d8aa0>

Here, you can see they exist in the same place in memory. Wrapping them in the _build function and then calling the fixture actually generates two distinct recipe objects. In doing this, the above assert passes.

sbillinge · 2026-03-02T17:19:00Z

yes, this is what I am worried about. It means are tests are brittle and not well designed. We need to think more carefully about this. If you can't trust your test, you are lost.

…dd a new helper function that builds recipe for init testing

cadenmyers13 · 2026-03-02T19:30:15Z

@sbillinge I do feel confident the fixture is operating as expected, but we are changing this one fixture for just two functions. Instead, I added a new helper function that builds two recipe objects called build_recipe_for_init_testing(). This function is only used for the two tests.

sbillinge · 2026-03-03T00:04:10Z

Rather than copy paste the function, I wonder if it makes sense to have the function in conftest.py and call it twice in the fixture, then return two different instances of the recipe in the fixture, something like that?

cadenmyers13 · 2026-03-03T01:35:26Z

@sbillinge Yeah, good idea. Less confusing for future devs, too. Added that now

cadenmyers13 · 2026-03-03T13:35:47Z

@sbillinge ready for review👍

sbillinge

nearly there. Small tweak.

sbillinge · 2026-03-03T14:57:12Z

tests/conftest.py

    return recipe


+@pytest.fixture(scope="function")


This is good, but we don't need to repeat it. Just have build_recipes_for testing that returns two recipes, then when it is used,

recipe, _ = build_recipes()

or

recipe1, recipe2 = build_recipes()

if you want both.

cadenmyers13 · 2026-03-03T16:15:14Z

@sbillinge ready for review. I made the preexisting fixture return two fit recipes instead of one.

sbillinge

small request, but could we call it build_recipes_one_contribution to better reflect what it actually does? thanks so much.

cadenmyers13 · 2026-03-03T20:50:27Z

@sbillinge sure thing! done

sbillinge · 2026-03-03T20:57:30Z

phew, that was a bit of a struggle but I think worth it in the end.

cadenmyers13 · 2026-03-03T21:03:33Z

@sbillinge agreed lol. thanks for the help

cadenmyers13 added 8 commits February 26, 2026 14:35

fix conftest fixture so it creates new recipe object on each call

698842e

update fixture initial values to be more different

12ee91c

update expected to accound for the slight difference now

0d6e1be

feat: initialize FitRecipe from a results file or object

474b32e

test: tests for the new feature

96ac076

add utils objects to get dict from results object or file

66a658d

news

83fd535

fix accident in test_fitresults

0c25030

cadenmyers13 commented Feb 26, 2026

View reviewed changes

cadenmyers13 added 3 commits February 26, 2026 15:49

use get_results_dictionary method to get params_dict

0625461

initializeRecipe deprecation

17f2b10

add better docstrings to fitrecipe and fitresults

9e205d7

sbillinge reviewed Feb 26, 2026

View reviewed changes

add 'The' to docstrings

39178a6

sbillinge reviewed Feb 27, 2026

View reviewed changes

do linspace with odd number of points

73704f4

cadenmyers13 added 2 commits March 2, 2026 14:20

revert recipe fixture back, explicitly set scope to 'function', and a…

aae48e0

…dd a new helper function that builds recipe for init testing

add assert recipes arent equal

02b0443

add fixture that returns two identical recipes

4c4b066

sbillinge reviewed Mar 3, 2026

View reviewed changes

add second recipe to fixture build_recipe_one_contribution

70dbe78

sbillinge reviewed Mar 3, 2026

View reviewed changes

change fixture name to be plural (build_recipes_one_contribution)

08478d4

sbillinge merged commit 89a5558 into diffpy:v3.3.0 Mar 3, 2026
4 checks passed

cadenmyers13 deleted the init-w-results branch March 3, 2026 21:03

cadenmyers13 mentioned this pull request Mar 3, 2026

Deprecate and refactor: Deprecate and refactor initializeResults and resultsDictionary #161

Closed

Conversation

cadenmyers13 commented Feb 26, 2026

Uh oh!

cadenmyers13 commented Feb 26, 2026

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!

cadenmyers13 Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge 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!

cadenmyers13 commented Feb 26, 2026

Uh oh!

codecov bot commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cadenmyers13 commented Feb 27, 2026

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Feb 27, 2026

Uh oh!

sbillinge commented Feb 27, 2026

Uh oh!

cadenmyers13 commented Mar 2, 2026

Uh oh!

sbillinge commented Mar 2, 2026

Uh oh!

cadenmyers13 commented Mar 2, 2026

Uh oh!

sbillinge commented Mar 3, 2026

Uh oh!

cadenmyers13 commented Mar 3, 2026

Uh oh!

cadenmyers13 commented Mar 3, 2026

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Mar 3, 2026

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Mar 3, 2026

Uh oh!

Uh oh!

sbillinge commented Mar 3, 2026

cadenmyers13 Feb 27, 2026 •

edited

Loading

cadenmyers13 Feb 27, 2026 •

edited

Loading

codecov bot commented Feb 27, 2026 •

edited

Loading