-
Notifications
You must be signed in to change notification settings - Fork 24
fix: fix iterpars behavior #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
9dbf55e
5a81792
da59981
2878173
cb6ab71
0b16bfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| **Added:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Changed:** | ||
|
|
||
| * Changed `iterPars` method to match all equal-type atoms to have same ADPs | ||
|
|
||
| **Deprecated:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Removed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Fixed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Security:** | ||
|
|
||
| * <news item> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -240,7 +240,9 @@ def _iter_managed(self): | |
| """Get iterator over managed objects.""" | ||
| return chain(*(d.values() for d in self.__managed)) | ||
|
|
||
| def iterate_over_parameters(self, pattern="", recurse=True): | ||
| def iterate_over_parameters( | ||
| self, pattern="", recurse=True, fullnames=False | ||
| ): | ||
| """Iterate over the Parameters contained in this object. | ||
|
|
||
| Parameters | ||
|
|
@@ -256,6 +258,10 @@ def iterate_over_parameters(self, pattern="", recurse=True): | |
| (default), the method will also iterate over | ||
| parameters in managed sub-objects. If False, only | ||
| top-level parameters will be iterated over. | ||
| fullnames : bool, optional | ||
| The flag indicating whether to match against hierarchical | ||
| dotted namesrelative to this object. | ||
| If False (default), match only leaf parameter names. | ||
|
|
||
| Example | ||
| ------- | ||
|
|
@@ -266,22 +272,74 @@ def iterate_over_parameters(self, pattern="", recurse=True): | |
| print(f"{param.name}={param.value}") | ||
| """ | ||
| regexp = re.compile(pattern) | ||
| if not fullnames: | ||
| yield from self._iterpars_leafnames(regexp, pattern, recurse) | ||
| return | ||
|
|
||
| yield from self._iterpars_fullnames(regexp, recurse=recurse, prefix="") | ||
|
|
||
| def _iter_local_parameters(self, regexp, prefix=""): | ||
| """Iterate over local Parameters with matching names.""" | ||
| for parameter in list(self._parameters.values()): | ||
| if regexp.search(parameter.name): | ||
| name = f"{prefix}{parameter.name}" | ||
| if regexp.search(name): | ||
| yield parameter | ||
| if not recurse: | ||
| return | ||
| # Iterate over objects within the managed dictionaries. | ||
|
|
||
| def _iter_managed_parameter_containers(self): | ||
| """Iterate over managed objects that can iterate over | ||
| Parameters.""" | ||
| managed = self.__managed[:] | ||
| managed.remove(self._parameters) | ||
| for m in managed: | ||
| for obj in m.values(): | ||
|
|
||
| for managed_dict in managed: | ||
| for obj in managed_dict.values(): | ||
| if hasattr(obj, "iterate_over_parameters"): | ||
| for parameter in obj.iterate_over_parameters( | ||
| pattern=pattern | ||
| ): | ||
| yield parameter | ||
| return | ||
| yield obj | ||
|
|
||
| def _iterpars_leafnames(self, regexp, pattern, recurse=True): | ||
| """Iterate over Parameters matched by leaf parameter names.""" | ||
| yield from self._iter_local_parameters(regexp) | ||
|
|
||
| if not recurse: | ||
| return | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you go through and delete unnecessary blank lines in the source code? Its okay to have this in tests but please remove in src |
||
| for obj in self._iter_managed_parameter_containers(): | ||
| yield from obj.iterate_over_parameters( | ||
| pattern=pattern, | ||
| recurse=True, | ||
| ) | ||
|
|
||
| def _iter_child_fullname_parameters(self, obj, regexp, prefix): | ||
| """Iterate over one child's Parameters with hierarchical | ||
| names.""" | ||
| if hasattr(obj, "_iterpars_fullnames"): | ||
| childprefix = f"{prefix}{obj.name}." | ||
| yield from obj._iterpars_fullnames( | ||
| regexp, | ||
| recurse=True, | ||
| prefix=childprefix, | ||
| ) | ||
| return | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. Not sure you need the |
||
|
|
||
| yield from obj.iterate_over_parameters( | ||
| pattern=regexp.pattern, | ||
| recurse=True, | ||
| ) | ||
|
|
||
| def _iterpars_fullnames(self, regexp, recurse=True, prefix=""): | ||
| """Iterate over Parameters matched by hierarchical dotted | ||
| names.""" | ||
| yield from self._iter_local_parameters(regexp, prefix=prefix) | ||
|
|
||
| if not recurse: | ||
| return | ||
|
|
||
| for obj in self._iter_managed_parameter_containers(): | ||
| yield from self._iter_child_fullname_parameters( | ||
| obj, | ||
| regexp, | ||
| prefix, | ||
| ) | ||
|
|
||
| @deprecated(iterPars_deprecation_msg) | ||
| def iterPars(self, pattern="", recurse=True): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,8 @@ | |
|
|
||
| from diffpy.srfit.exceptions import SrFitError | ||
| from diffpy.srfit.fitbase import ProfileParser | ||
| from diffpy.srfit.fitbase.parameter import Parameter | ||
| from diffpy.srfit.fitbase.recipeorganizer import RecipeContainer | ||
| from diffpy.srfit.pdf import PDFContribution, PDFGenerator, PDFParser | ||
|
|
||
| # ---------------------------------------------------------------------------- | ||
|
|
@@ -321,3 +323,114 @@ def test_pickling( | |
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
|
|
||
|
|
||
| def _make_iterpars_tree(): | ||
| """Build a small hierarchy for iterPars tests.""" | ||
| root = RecipeContainer("root") | ||
| root._containers = {} | ||
| root._manage(root._containers) | ||
|
|
||
| root_biso = Parameter("Biso", 10) | ||
| root._add_object(root_biso, root._parameters) | ||
|
|
||
| ni0 = RecipeContainer("Ni0") | ||
| ni0_biso = Parameter("Biso", 20) | ||
| ni0_uiso = Parameter("Uiso", 30) | ||
| ni0._add_object(ni0_biso, ni0._parameters) | ||
| ni0._add_object(ni0_uiso, ni0._parameters) | ||
|
|
||
| ni1 = RecipeContainer("Ni1") | ||
| ni1_biso = Parameter("Biso", 40) | ||
| ni1._add_object(ni1_biso, ni1._parameters) | ||
|
|
||
| o0 = RecipeContainer("O0") | ||
| o0_biso = Parameter("Biso", 50) | ||
| o0._add_object(o0_biso, o0._parameters) | ||
|
|
||
| root._add_object(ni0, root._containers) | ||
| root._add_object(ni1, root._containers) | ||
| root._add_object(o0, root._containers) | ||
|
|
||
| return { | ||
| "root": root, | ||
| "root_biso": root_biso, | ||
| "ni0": ni0, | ||
| "ni0_biso": ni0_biso, | ||
| "ni0_uiso": ni0_uiso, | ||
| "ni1": ni1, | ||
| "ni1_biso": ni1_biso, | ||
| "o0": o0, | ||
| "o0_biso": o0_biso, | ||
| } | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("pattern", "kwargs", "expected_values"), | ||
| [ | ||
| # C1: Match leaf parameter names without fullnames. | ||
| # Expected: all Biso parameters in the hierarchy are returned. | ||
| (r"^Biso$", {}, [10, 20, 40, 50]), | ||
| # C2: Match hierarchical names without fullnames. | ||
| # Expected: no leaf names match the hierarchical pattern. | ||
| (r"^Ni\d+\.Biso$", {}, []), | ||
| # C3: Match hierarchical names with fullnames enabled. | ||
| # Expected: matching Ni Biso parameters are returned. | ||
| (r"^Ni\d+\.Biso$", {"fullnames": True}, [20, 40]), | ||
| # C4: Match one hierarchical Uiso name. | ||
| # Expected: only Ni0.Uiso is returned. | ||
| (r"^Ni0\.Uiso$", {"fullnames": True}, [30]), | ||
| # C5: Match one hierarchical Biso name outside Ni containers. | ||
| # Expected: only O0.Biso is returned. | ||
| (r"^O0\.Biso$", {"fullnames": True}, [50]), | ||
| # C6: Disable recursion while matching child fullnames. | ||
| # Expected: no child parameters are returned. | ||
| (r"^Ni\d+\.Biso$", {"fullnames": True, "recurse": False}, []), | ||
| # C7: Disable recursion while matching root fullname. | ||
| # Expected: only the root-level Biso parameter is returned. | ||
| (r"^Biso$", {"fullnames": True, "recurse": False}, [10]), | ||
| ], | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add comments here explaining what each test case is? This will look something like |
||
| def test_iterpars_fullname_matching(pattern, kwargs, expected_values): | ||
| """Verify leaf-name and fullname matching in | ||
| iterate_over_parameters.""" | ||
| objs = _make_iterpars_tree() | ||
| root = objs["root"] | ||
|
|
||
| actual_values = [ | ||
| parameter.value | ||
| for parameter in root.iterate_over_parameters(pattern, **kwargs) | ||
| ] | ||
|
|
||
| assert actual_values == expected_values | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("pattern", "expected_name"), | ||
| [ | ||
| # C1: Match Biso relative to the called Ni0 container. | ||
| # Expected: Ni0.Biso is returned without the Ni0 prefix. | ||
| (r"^Biso$", ["Biso"]), | ||
| # C2: Match Uiso relative to the called Ni0 container. | ||
| # Expected: Ni0.Uiso is returned without the Ni0 prefix. | ||
| (r"^Uiso$", ["Uiso"]), | ||
| # C3: Match with the parent container prefix from inside Ni0. | ||
| # Expected: no parameter is returned because fullnames are relative | ||
| # to the container on which iterate_over_parameters is called. | ||
| (r"^Ni0\.Biso$", []), | ||
| ], | ||
| ) | ||
| def test_iterpars_fullnames_are_relative_to_called_container( | ||
| pattern, | ||
| expected_name, | ||
| ): | ||
| """Verify fullname matching is relative to the called container.""" | ||
| objs = _make_iterpars_tree() | ||
| ni0 = objs["ni0"] | ||
|
|
||
| actual_name = [ | ||
| parameter.name | ||
| for parameter in ni0.iterate_over_parameters(pattern, fullnames=True) | ||
| ] | ||
|
|
||
| assert actual_name == expected_name | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its more common to do
ifelseinstead of havingreturnin the conditional. So it would look like,or you might be able to just delete
returnThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the former approach works but the latter one is not right. If we only delete the
return, after the firstyieldstatement is executed, it would keep on executing the secondyield, which is not right.