diff --git a/news/fix-restraints.rst b/news/fix-restraints.rst new file mode 100644 index 00000000..f6039ab0 --- /dev/null +++ b/news/fix-restraints.rst @@ -0,0 +1,23 @@ +**Added:** + +* + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* Apply restraints and constraints added to a ``FitContribution`` or ``ParameterSet`` after its ``FitRecipe`` has been evaluated. Before, they were ignored until the recipe was reset. + +**Security:** + +* diff --git a/src/diffpy/srfit/fitbase/configurable.py b/src/diffpy/srfit/fitbase/configurable.py index 7c0ba19d..1dc27530 100644 --- a/src/diffpy/srfit/fitbase/configurable.py +++ b/src/diffpy/srfit/fitbase/configurable.py @@ -42,15 +42,24 @@ def _update_configuration(self): return def _store_configurable(self, obj): - """Store a Configurable. + """Store a Configurable to notify of configuration changes. The passed obj is only stored if it is a Configurable, otherwise - this method quietly exits. + this method quietly exits. Containers store themselves in the + objects they hold, so that changes are passed up the hierarchy. """ if isinstance(obj, Configurable): self._configobjs.add(obj) return + def _remove_configurable(self, obj): + """Stop notifying a stored Configurable of changes. + + The passed obj is ignored if it is not stored. + """ + self._configobjs.discard(obj) + return + # End class Configurable diff --git a/src/diffpy/srfit/fitbase/recipeorganizer.py b/src/diffpy/srfit/fitbase/recipeorganizer.py index 37eef2b6..cbbe4d42 100644 --- a/src/diffpy/srfit/fitbase/recipeorganizer.py +++ b/src/diffpy/srfit/fitbase/recipeorganizer.py @@ -628,6 +628,8 @@ def _add_object(self, obj, d, check=True): # Detach the old object, if there is one if oldobj is not None: oldobj.remove_observer(self._flush) + if isinstance(oldobj, Configurable): + oldobj._remove_configurable(self) # Add the object d[obj.name] = obj @@ -635,8 +637,10 @@ def _add_object(self, obj, d, check=True): # Observe the object obj.add_observer(self._flush) - # Store this as a configurable object - self._store_configurable(obj) + # Pass configuration changes in the object up to this one + if isinstance(obj, Configurable): + obj._store_configurable(self) + self._update_configuration() return def _remove_object(self, obj, d): @@ -653,6 +657,9 @@ def _remove_object(self, obj, d): del d[obj.name] obj.remove_observer(self._flush) + if isinstance(obj, Configurable): + obj._remove_configurable(self) + self._update_configuration() return diff --git a/src/diffpy/srfit/structure/objcrystparset.py b/src/diffpy/srfit/structure/objcrystparset.py index fc09ba7b..caaf45d7 100644 --- a/src/diffpy/srfit/structure/objcrystparset.py +++ b/src/diffpy/srfit/structure/objcrystparset.py @@ -358,15 +358,15 @@ def wrapRestraints(self): # from within this class. for b in self.scat.GetBondList(): res = ObjCrystMoleculeRestraint(b) - self._restraints.add(res) + self.register_soft_bounds(res) for ba in self.scat.GetBondAngleList(): res = ObjCrystMoleculeRestraint(ba) - self._restraints.add(res) + self.register_soft_bounds(res) for da in self.scat.GetDihedralAngleList(): res = ObjCrystMoleculeRestraint(da) - self._restraints.add(res) + self.register_soft_bounds(res) return @@ -463,7 +463,7 @@ def restrainBondLength( res = ObjCrystBondLengthRestraint( atom1, atom2, length, sigma, delta, scaled ) - self._restraints.add(res) + self.register_soft_bounds(res) return res @@ -537,7 +537,7 @@ def restrainBondAngle( res = ObjCrystBondAngleRestraint( atom1, atom2, atom3, angle, sigma, delta, scaled ) - self._restraints.add(res) + self.register_soft_bounds(res) return res @@ -612,7 +612,7 @@ def restrainDihedralAngle( res = ObjCrystDihedralAngleRestraint( atom1, atom2, atom3, atom4, angle, sigma, delta, scaled ) - self._restraints.add(res) + self.register_soft_bounds(res) return res diff --git a/tests/test_fitrecipe.py b/tests/test_fitrecipe.py index e8af724c..761dbf52 100644 --- a/tests/test_fitrecipe.py +++ b/tests/test_fitrecipe.py @@ -32,6 +32,7 @@ ProfileParser, ) from diffpy.srfit.fitbase.parameter import Parameter +from diffpy.srfit.fitbase.parameterset import ParameterSet matplotlib.use("Agg") @@ -327,6 +328,104 @@ def test_residual_with_contribution_constraints_and_restraints( assert actual_final_residual == pytest.approx(expected_final_residual) +def _get_restraint_bounds(restraints): + """Return the sorted (lower_bound, upper_bound, sig) of each + restraint.""" + sorted_restraints = sorted( + (restraint.lower_bound, restraint.upper_bound, restraint.sig) + for restraint in restraints + ) + return sorted_restraints + + +def test_add_bounds_after_recipe_evaluation(): + # Test that soft bounds added to a contribution after the recipe is + # evaluated are used in the next evaluation. + profile = Profile() + profile.set_observed_profile(linspace(0, 1, 5), ones(5)) + contribution = FitContribution("contrib") + contribution.set_profile(profile) + contribution.set_equation("m*x + b") + # C1: m is restrained in the contribution before it is added to a recipe. + # Expected: The contribution holds the restraint on m. + contribution.add_soft_bounds("m", lower_bound=0, upper_bound=2, sig=1) + actual_bounds_before_eval = _get_restraint_bounds( + contribution._get_restraints() + ) + expected_bounds_before_eval = [(0, 2, 1)] + assert actual_bounds_before_eval == expected_bounds_before_eval + # C2: The recipe is evaluated, then b is restrained in the contribution + # and the recipe is evaluated again. + # Expected: The recipe uses the restraints on both m and b. + recipe = FitRecipe() + recipe.add_contribution(contribution) + recipe.add_variable(contribution.m, 1) + recipe.add_variable(contribution.b, 0) + # Evaluate the recipe. + recipe.residual() + recipe.contrib.add_soft_bounds("b", lower_bound=-1, upper_bound=1, sig=3) + # Evaluate the recipe again to include the new restraint. + recipe.residual() + actual_bounds_after_eval = _get_restraint_bounds(recipe._restraintlist) + expected_bounds_after_eval = [(-1, 1, 3), (0, 2, 1)] + assert actual_bounds_after_eval == expected_bounds_after_eval + + +def test_add_bounds_to_nested_parameter_set_after_recipe_evaluation( + build_sine_recipe, +): + # Test that soft bounds added to a ParameterSet nested two levels below + # the recipe after the recipe is evaluated are used in the next + # evaluation. + outer = ParameterSet("outer") + inner = ParameterSet("inner") + inner.new_parameter("p", 1) + inner.new_parameter("q", 1) + # C1: p is restrained in "inner" before "inner" is added to a recipe. + # Expected: "inner" holds the restraint on p. + inner.add_soft_bounds("p", lower_bound=0, upper_bound=2, sig=1) + actual_bounds_before_eval = _get_restraint_bounds(inner._get_restraints()) + expected_bounds_before_eval = [(0, 2, 1)] + assert actual_bounds_before_eval == expected_bounds_before_eval + # C2: The recipe holding "outer", which holds "inner", is evaluated, then + # q is restrained in "inner" and the recipe is evaluated again. + # Expected: The recipe uses the restraints on both p and q. + recipe = build_sine_recipe() + outer.add_parameter_set(inner) + recipe.add_parameter_set(outer) + # Evaluate the recipe. + recipe.residual() + recipe.outer.inner.add_soft_bounds( + "q", lower_bound=-1, upper_bound=1, sig=3 + ) + # Evaluate the recipe again to include the new restraint. + recipe.residual() + actual_bounds_after_eval = _get_restraint_bounds(recipe._restraintlist) + expected_bounds_after_eval = [(-1, 1, 3), (0, 2, 1)] + assert actual_bounds_after_eval == expected_bounds_after_eval + + +def test_add_constraint_after_recipe_evaluation(build_sine_recipe): + # Test that a constraint added to a contribution after the recipe is + # evaluated is updated in the next evaluations. + recipe = build_sine_recipe() + recipe.add_variable(recipe.cont.A) + # Evaluate the recipe. + recipe.residual() + # C1: c is constrained to 2*A in the contribution with A = 1. + # Expected: c is 2. + recipe.cont.add_constraint(recipe.cont.c, "2*A") + actual_c_value = recipe.cont.c.value + expected_c_value = 2 + assert actual_c_value == expected_c_value + # C2: The recipe is evaluated again with A = 3. + # Expected: c follows A and is 6. + recipe.residual([3]) + actual_c_value = recipe.cont.c.value + expected_c_value = 6 + assert actual_c_value == expected_c_value + + def test_convert_bounds_to_restraints(): # C1: A variable bounded to [-1, 1] is converted with sig=2 and # scaled=True.