Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions news/iterpar-behavior.rst
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>
49 changes: 43 additions & 6 deletions src/diffpy/srfit/fitbase/recipeorganizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -256,6 +258,9 @@ 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
Match against hierarchical dotted names relative to this object
when True. Match only leaf parameter names when False (default).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this description begin with "The"?


Example
-------
Expand All @@ -266,22 +271,54 @@ def iterate_over_parameters(self, pattern="", recurse=True):
print(f"{param.name}={param.value}")
"""
regexp = re.compile(pattern)
if not fullnames:
for parameter in list(self._parameters.values()):
if regexp.search(parameter.name):
yield parameter
if not recurse:
return
managed = self.__managed[:]
managed.remove(self._parameters)
for m in managed:
for obj in m.values():
if hasattr(obj, "iterate_over_parameters"):
for parameter in obj.iterate_over_parameters(
pattern=pattern, recurse=True
):
yield parameter
return
Copy link
Copy Markdown
Contributor

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 if else instead of having return in the conditional. So it would look like,

if not fullnames:
   yield ...
else:
   yield ...

or you might be able to just delete return

Copy link
Copy Markdown
Contributor Author

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 first yield statement is executed, it would keep on executing the second yield, which is not right.

for parameter in self._iterpars_fullnames(
regexp, recurse=recurse, prefix=""
):
yield parameter
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these nested for loops and conditionals makes it hard to read. Can you break these out into private functions that do all the for loop work so when a condition is met we just call the private function? Each private function should be designed to do one thing then we call all the private functions within iterate_over_parameters


def _iterpars_fullnames(self, regexp, recurse=True, prefix=""):
"""Internal helper for iterPars(fullnames=True)."""
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

managed = self.__managed[:]
managed.remove(self._parameters)
for m in managed:
for obj in m.values():
if hasattr(obj, "iterate_over_parameters"):
if hasattr(obj, "_iterpars_fullnames"):
childprefix = f"{prefix}{obj.name}."
for parameter in obj._iterpars_fullnames(
regexp,
recurse=True,
prefix=childprefix,
):
yield parameter
elif hasattr(obj, "iterate_over_parameters"):
for parameter in obj.iterate_over_parameters(
pattern=pattern
pattern=regexp.pattern, recurse=True
):
yield parameter
return

@deprecated(iterPars_deprecation_msg)
def iterPars(self, pattern="", recurse=True):
Expand Down
83 changes: 83 additions & 0 deletions tests/test_pdf.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

# ----------------------------------------------------------------------------
Expand Down Expand Up @@ -321,3 +323,84 @@ 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"),
[
(r"^Biso$", {}, [10, 20, 40, 50]),
(r"^Ni\d+\.Biso$", {}, []),
(r"^Ni\d+\.Biso$", {"fullnames": True}, [20, 40]),
(r"^Ni0\.Uiso$", {"fullnames": True}, [30]),
(r"^O0\.Biso$", {"fullnames": True}, [50]),
(r"^Ni\d+\.Biso$", {"fullnames": True, "recurse": False}, []),
(r"^Biso$", {"fullnames": True, "recurse": False}, [10]),
],
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

@pytest.mark.parametrize(
    ("pattern", "kwargs", "expected_values"),
    [
        # C1: user inputs something
        # expected: the expected behavior is this
        (r"^Biso$", {}, [10, 20, 40, 50]),
...

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"]

values = [
par.value for par in root.iterate_over_parameters(pattern, **kwargs)
]

assert values == expected_values
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this actual_values == expected_values? Just for consistency



def test_iterpars_fullnames_are_relative_to_called_container():
"""Verify fullname matching is relative to the called container."""
objs = _make_iterpars_tree()
ni0 = objs["ni0"]

assert list(ni0.iterate_over_parameters(r"^Biso$", fullnames=True)) == [
objs["ni0_biso"]
]
assert list(ni0.iterate_over_parameters(r"^Uiso$", fullnames=True)) == [
objs["ni0_uiso"]
]
assert (
list(ni0.iterate_over_parameters(r"^Ni0\.Biso$", fullnames=True)) == []
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my above comment. Break these out and store them as variables so the assert statement says actual_<whatever> == expected_<whatever>. This makes interpreting tests much easier

Loading