diff --git a/CODE-OF-CONDUCT.rst b/CODE-OF-CONDUCT.rst index d0115c8..ee7326f 100644 --- a/CODE-OF-CONDUCT.rst +++ b/CODE-OF-CONDUCT.rst @@ -67,7 +67,7 @@ Enforcement Instances of abusive, harassing, or otherwise unacceptable behavior may be reported to the community leaders responsible for enforcement at -sbillinge@ucsb.edu and cadenmyers13@columbia.edu. All complaints will be reviewed and investigated promptly and fairly. +sbillinge@ucsb.edu and cjm2304@columbia.edu. All complaints will be reviewed and investigated promptly and fairly. All community leaders are obligated to respect the privacy and security of the reporter of any incident. diff --git a/README.rst b/README.rst index b145c35..9522d27 100644 --- a/README.rst +++ b/README.rst @@ -129,7 +129,7 @@ Before contributing, please read our `Code of Conduct `_ or email the maintainers ``Simon Billinge(sbillinge@ucsb.edu) and Caden Myers(cadenmyers13@columbia.edu)``. +For more information on diffpy.cmirietveld please visit the project `web-page `_ or email the maintainers ``Simon Billinge(sbillinge@ucsb.edu) and Caden Myers(cjm2304@columbia.edu)``. Acknowledgements ---------------- diff --git a/cookiecutter.json b/cookiecutter.json index 3d0def0..80f07a4 100644 --- a/cookiecutter.json +++ b/cookiecutter.json @@ -1,8 +1,8 @@ { "author_names": "Simon Billinge, Caden Myers", - "author_emails": "sbillinge@ucsb.edu, cadenmyers13@columbia.edu", + "author_emails": "sbillinge@ucsb.edu, cjm2304@columbia.edu", "maintainer_names": "Simon Billinge, Caden Myers", - "maintainer_emails": "sbillinge@ucsb.edu, cadenmyers13@columbia.edu", + "maintainer_emails": "sbillinge@ucsb.edu, cjm2304@columbia.edu", "maintainer_github_usernames": "sbillinge", "contributors": "Caden Myers, members of the Billinge group, and contributors to the diffpy.cmirietveld project", "project_name": "diffpy.cmirietveld", diff --git a/news/component-parameters.rst b/news/component-parameters.rst new file mode 100644 index 0000000..a0489b6 --- /dev/null +++ b/news/component-parameters.rst @@ -0,0 +1,23 @@ +**Added:** + +* Add ``Component.create_parameters_from``, which creates a component's Parameters from a pyobjcryst model, with the model's default values and its limits as bounds, and ``Component.push_parameters``, which writes the values back and raises a ``ValueError`` for a value outside the limits instead of letting ObjCryst clamp it silently. + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* + +**Security:** + +* diff --git a/pyproject.toml b/pyproject.toml index 741f640..52491de 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -7,11 +7,11 @@ name = "diffpy.cmirietveld" dynamic=['version', 'dependencies'] authors = [ {name='Simon Billinge', email='sbillinge@ucsb.edu'}, - {name='Caden Myers', email='cadenmyers13@columbia.edu'}, + {name='Caden Myers', email='cjm2304@columbia.edu'}, ] maintainers = [ {name='Simon Billinge', email='sbillinge@ucsb.edu'}, - {name='Caden Myers', email='cadenmyers13@columbia.edu'}, + {name='Caden Myers', email='cjm2304@columbia.edu'}, ] description = "A pack for diffpy.cmi that allows it to do Rietveld refinement of powder diffraction data" keywords = ['diffraction', 'PDF', 'X-ray', 'neutron', 'Rietveld'] diff --git a/requirements/conda.txt b/requirements/conda.txt index eacb3e1..f69c4a0 100644 --- a/requirements/conda.txt +++ b/requirements/conda.txt @@ -1,2 +1,3 @@ numpy diffpy.srfit +pyobjcryst diff --git a/requirements/pip.txt b/requirements/pip.txt index eacb3e1..f69c4a0 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -1,2 +1,3 @@ numpy diffpy.srfit +pyobjcryst diff --git a/src/diffpy/cmirietveld/component.py b/src/diffpy/cmirietveld/component.py index fbab91c..de5cb74 100644 --- a/src/diffpy/cmirietveld/component.py +++ b/src/diffpy/cmirietveld/component.py @@ -22,6 +22,7 @@ __all__ = ["Component"] +from diffpy.srfit.fitbase.parameter import Parameter from diffpy.srfit.fitbase.parameterset import ParameterSet @@ -38,6 +39,10 @@ class Component(ParameterSet): The name of the component. It must be a valid attribute name. """ + def __init__(self, name): + ParameterSet.__init__(self, name) + self._refinable_par_names = {} + def install(self, pattern, diffraction): """Set the model up in one generator. @@ -73,20 +78,81 @@ def create_parameters_from(self, refinable_obj, names): model. Each Parameter takes the default value of the matching - ``RefinablePar`` and its limits as bounds. + ``RefinablePar`` and its limits as bounds. ObjCryst silently clamps + a value outside its limits, so the bounds keep a refinement within + the values ObjCryst actually uses. + + The Parameter names are mapped to the ``RefinablePar`` names + rather than copied, so the names users see are this package's, + independent of ObjCryst's. Some ObjCryst names are not valid + Parameter names (``2ThetaDispl``). The mapping keeps ObjCryst + parameters out of refinement scripts. The mapping is stored, so + ``push_parameters`` writes each value back to the same + ``RefinablePar``. Parameters ---------- refinable_obj : pyobjcryst.refinableobj.RefinableObj A fresh instance of the pyobjcryst model. names : dict of str to str - The ``RefinablePar`` name for each Parameter name. + The ``RefinablePar`` name for each Parameter name, for example + ``{"eta0": "Eta0"}``. Only the named ``RefinablePar``s get a + Parameter. + + Raises + ------ + ValueError + If the model has no ``RefinablePar`` with one of the names. + + Examples + -------- + In the ``__init__`` of a peak profile: + + >>> self.create_parameters_from( + ... ReflectionProfilePseudoVoigt(), + ... {"U": "U", "V": "V", "W": "W", "eta0": "Eta0"}, + ... ) """ - raise NotImplementedError + available_names = [ + refinable_obj.GetPar(index).GetName() + for index in range(refinable_obj.GetNbPar()) + ] + for refinable_par_name in names.values(): + if refinable_par_name not in available_names: + raise ValueError( + f"{type(refinable_obj).__name__} has no refinable " + f"parameter named '{refinable_par_name}'. Please use one " + f"of its parameter names: {', '.join(available_names)}." + ) + for parameter_name, refinable_par_name in names.items(): + refinable_par = refinable_obj.GetPar(refinable_par_name) + parameter = Parameter( + parameter_name, refinable_par.GetHumanValue() + ) + if refinable_par.IsLimited(): + parameter.bound_range( + refinable_par.GetHumanMin(), refinable_par.GetHumanMax() + ) + self.add_parameter(parameter) + self._refinable_par_names.update(names) def push_parameters(self, refinable_obj): """Push the Parameter values into a pyobjcryst model. + Each value is written to the ``RefinablePar`` that + ``create_parameters_from`` mapped it to. + + A value outside the limits of its ``RefinablePar`` raises an error + instead of being clamped, because ObjCryst would otherwise use a + different value from the one the Parameter reports. The check uses + ObjCryst's limits, not the Parameter's bounds: a value outside + narrowed bounds is still pushed, and widened bounds don't let a + value past a limit. An optimizer run without the recipe's bounds + stops with this error the first time it steps past a limit. + + All the values are checked before any is pushed, so an error + leaves the model unchanged rather than half updated. + Parameters ---------- refinable_obj : pyobjcryst.refinableobj.RefinableObj @@ -95,9 +161,38 @@ def push_parameters(self, refinable_obj): Raises ------ ValueError - If a Parameter value is outside its bounds. + If a Parameter value is outside the limits of its + ``RefinablePar``. """ - raise NotImplementedError + values = { + parameter_name: self.get(parameter_name).value + for parameter_name in self._refinable_par_names + } + refinable_pars = { + parameter_name: refinable_obj.GetPar(refinable_par_name) + for parameter_name, refinable_par_name in ( + self._refinable_par_names.items() + ) + } + for parameter_name, refinable_par in refinable_pars.items(): + if not refinable_par.IsLimited(): + continue + value = values[parameter_name] + lower_limit = refinable_par.GetHumanMin() + upper_limit = refinable_par.GetHumanMax() + if not lower_limit <= value <= upper_limit: + used_value = min(max(value, lower_limit), upper_limit) + raise ValueError( + f"The value of '{parameter_name}', {value}, is outside " + f"its limits [{lower_limit}, {upper_limit}]. ObjCryst " + f"would silently use {used_value} instead, so the refined " + "value would not be the one used to calculate the " + "pattern. Please keep the value within the limits, for " + "example by passing the recipe's bounds to the optimizer: " + "least_squares(..., bounds=recipe.get_bounds_array())." + ) + for parameter_name, refinable_par in refinable_pars.items(): + refinable_par.SetHumanValue(values[parameter_name]) # End of file diff --git a/tests/conftest.py b/tests/conftest.py index e3b6313..f2b35a3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,6 +2,43 @@ from pathlib import Path import pytest +from pyobjcryst.reflectionprofile import ReflectionProfilePseudoVoigt + +from diffpy.cmirietveld.component import Component + +PSEUDO_VOIGT_PARAMETER_NAMES = { + "U": "U", + "V": "V", + "W": "W", + "eta0": "Eta0", + "eta1": "Eta1", + "asym0": "Asym0", + "asym1": "Asym1", + "asym2": "Asym2", +} + + +@pytest.fixture +def pseudo_voigt_profile(): + profile = ReflectionProfilePseudoVoigt() + return profile + + +@pytest.fixture +def installed_pseudo_voigt_profile(): + """Return a profile separate from the one the component's Parameters + are created from, like the copy a generator installs.""" + profile = ReflectionProfilePseudoVoigt() + return profile + + +@pytest.fixture +def pseudo_voigt_component(pseudo_voigt_profile): + component = Component("peak_profile") + component.create_parameters_from( + pseudo_voigt_profile, PSEUDO_VOIGT_PARAMETER_NAMES + ) + return component @pytest.fixture diff --git a/tests/test_component.py b/tests/test_component.py new file mode 100644 index 0000000..72787b4 --- /dev/null +++ b/tests/test_component.py @@ -0,0 +1,193 @@ +import re + +import pytest + +from diffpy.cmirietveld.component import Component + + +def test_create_parameters_from_names(pseudo_voigt_component): + # Test that each name in the mapping becomes a Parameter + # Expected: The component holds one Parameter per name, and no others. + actual_names = sorted(pseudo_voigt_component.get_names()) + expected_names = [ + "U", + "V", + "W", + "asym0", + "asym1", + "asym2", + "eta0", + "eta1", + ] + assert actual_names == expected_names + + +@pytest.mark.parametrize( + "parameter_name, expected_value, expected_bounds", + [ + # Test that each Parameter takes the RefinablePar's human value + # and limits + # C1: The Caglioti W term is created from the pseudo-Voigt profile. + # Expected: Its value is ObjCryst's default, 0.01 deg^2, and its + # bounds are ObjCryst's limits, [0, 1]. + ("W", 0.01, [0.0, 1.0]), + # C2: The Caglioti U term, which can be negative, is created. + # Expected: Its value is 0 and its bounds are [-1, 1]. + ("U", 0.0, [-1.0, 1.0]), + # C3: The mixing term eta0 is created under a name that differs + # from its RefinablePar name, Eta0. + # Expected: Its value is 0.5 and its bounds are [0, 1]. + ("eta0", 0.5, [0.0, 1.0]), + # C4: The asymmetry term asym0, whose lower limit is not zero, is + # created. + # Expected: Its value is 1 and its bounds are [0.01, 10]. + ("asym0", 1.0, [0.01, 10.0]), + ], +) +def test_create_parameters_from_values_and_bounds( + pseudo_voigt_component, parameter_name, expected_value, expected_bounds +): + parameter = pseudo_voigt_component.get(parameter_name) + actual_value = parameter.value + actual_bounds = list(parameter.bounds) + assert actual_value == pytest.approx(expected_value) + assert actual_bounds == expected_bounds + + +def test_create_parameters_from_unknown_name(pseudo_voigt_profile): + # Test creating a Parameter from a RefinablePar the model doesn't have + # Expected: A ValueError names the missing RefinablePar and lists the + # names the model has. + component = Component("peak_profile") + expected_msg = ( + "ReflectionProfilePseudoVoigt has no refinable parameter named " + "'nonexistent_name'. Please use one of its parameter names: U, V, " + "W, Eta0, Eta1, Asym0, Asym1, Asym2." + ) + bad_input = {"any_name": "nonexistent_name"} + with pytest.raises(ValueError, match=re.escape(expected_msg)): + component.create_parameters_from(pseudo_voigt_profile, bad_input) + + +@pytest.mark.parametrize( + "parameter_name, refinable_par_name, new_value", + [ + # Test that Parameter values are pushed into a pyobjcryst model + # other than the one the Parameters were created from + # C1: The Caglioti W term is set inside its limits. + # Expected: The W RefinablePar has the new value. + ("W", "W", 0.05), + # C2: The mixing term eta0 is set under a name that differs from + # its RefinablePar name. + # Expected: The Eta0 RefinablePar has the new value. + ("eta0", "Eta0", 0.3), + # C3: The asymmetry term asym0 is set exactly at its upper limit. + # Expected: The Asym0 RefinablePar has the new value, because a + # value at a limit is inside the limits. + ("asym0", "Asym0", 10.0), + ], +) +def test_push_parameters( + pseudo_voigt_component, + installed_pseudo_voigt_profile, + parameter_name, + refinable_par_name, + new_value, +): + pseudo_voigt_component.get(parameter_name).set_value(new_value) + pseudo_voigt_component.push_parameters(installed_pseudo_voigt_profile) + actual_value = installed_pseudo_voigt_profile.GetPar( + refinable_par_name + ).GetHumanValue() + expected_value = new_value + assert actual_value == pytest.approx(expected_value) + + +@pytest.mark.parametrize( + "parameter_bounds, new_values, invalid_name, invalid_value, " + "expected_limits, expected_used_value", + [ + # Test pushing Parameter values when one is outside ObjCryst's + # limits + # C1: The Caglioti W term is set above its upper limit, with its + # bounds left at ObjCryst's limits. + # Expected: A ValueError names W, its limits and the value ObjCryst + # would use, and the model is unchanged. + ({}, {"W": 2.0}, "W", "2.0", "[0.0, 1.0]", "1.0"), + # C2: The asymmetry term asym0 is set below its lower limit, with its + # bounds left at ObjCryst's limits. + # Expected: A ValueError names asym0, its limits and the value + # ObjCryst would use, and the model is unchanged. + ({}, {"asym0": 0.001}, "asym0", "0.001", "[0.01, 10.0]", "0.01"), + # C3: The W term's bounds are widened past ObjCryst's limits to + # [0, 5], and W is set to 2, inside the bounds. + # Expected: The check uses ObjCryst's limits, not the Parameter's + # bounds, so a ValueError names the limits [0, 1], and the model is + # unchanged. + ({"W": [0.0, 5.0]}, {"W": 2.0}, "W", "2.0", "[0.0, 1.0]", "1.0"), + # C4: W, which comes first in the mapping, is set inside its limits, + # and asym0 is set above its upper limit, with both bounds left at + # ObjCryst's limits. + # Expected: A ValueError names asym0, and no value is pushed, not + # even W's, so the model is unchanged rather than half updated. + ( + {}, + {"W": 0.05, "asym0": 20.0}, + "asym0", + "20.0", + "[0.01, 10.0]", + "10.0", + ), + ], +) +def test_push_parameters_outside_limits( + pseudo_voigt_component, + installed_pseudo_voigt_profile, + parameter_bounds, + new_values, + invalid_name, + invalid_value, + expected_limits, + expected_used_value, +): + expected_msg = ( + f"The value of '{invalid_name}', {invalid_value}, is outside its " + f"limits {expected_limits}. ObjCryst would silently use " + f"{expected_used_value} instead, so the refined value would not be " + "the one used to calculate the pattern. Please keep the value " + "within the limits, for example by passing the recipe's bounds to " + "the optimizer: least_squares(..., bounds=recipe.get_bounds_array())." + ) + refinable_par_count = installed_pseudo_voigt_profile.GetNbPar() + expected_values = [ + installed_pseudo_voigt_profile.GetPar(index).GetHumanValue() + for index in range(refinable_par_count) + ] + for parameter_name, bounds in parameter_bounds.items(): + pseudo_voigt_component.get(parameter_name).bound_range(*bounds) + for parameter_name, new_value in new_values.items(): + pseudo_voigt_component.get(parameter_name).set_value(new_value) + with pytest.raises(ValueError, match=re.escape(expected_msg)): + pseudo_voigt_component.push_parameters(installed_pseudo_voigt_profile) + actual_values = [ + installed_pseudo_voigt_profile.GetPar(index).GetHumanValue() + for index in range(refinable_par_count) + ] + assert actual_values == expected_values + + +def test_push_parameters_outside_narrowed_bounds( + pseudo_voigt_component, installed_pseudo_voigt_profile +): + # Test that a value outside narrowed bounds but inside ObjCryst's + # limits is pushed + # Expected: W = 0.5 is outside the narrowed bounds [0, 0.1] but inside + # ObjCryst's limits [0, 1], so the W RefinablePar has the new value. + # Keeping a fit inside the bounds is the optimizer's job. + parameter = pseudo_voigt_component.get("W") + parameter.bound_range(0.0, 0.1) + parameter.set_value(0.5) + pseudo_voigt_component.push_parameters(installed_pseudo_voigt_profile) + actual_value = installed_pseudo_voigt_profile.GetPar("W").GetHumanValue() + expected_value = 0.5 + assert actual_value == pytest.approx(expected_value)