ComponentCollection #56

henrikjacobsenfys · 2025-10-17T09:08:23Z

Adding SampleModel which can contain ModelComponents and calculate the model at given x, with and without detailed balance.

henrikjacobsenfys · 2025-10-17T09:10:18Z

Code coverage is not 100%, but I think all the essentials are there, and I'd rather get feedback on the important things than polish tests that may need to change anyway :)

Edit: apparently codecov is upset about the missing tests, so I added tests and discovered a minor bug in the process.

codecov · 2025-10-17T09:10:47Z

Codecov Report

❌ Patch coverage is 99.66443% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.67%. Comparing base (01b9490) to head (23c8660).

Files with missing lines	Patch %	Lines
.../easydynamics/sample_model/component_collection.py	99.08%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #56      +/-   ##
===========================================
+ Coverage    96.60%   97.67%   +1.06%     
===========================================
  Files           12       13       +1     
  Lines          412      644     +232     
===========================================
+ Hits           398      629     +231     
- Misses          14       15       +1

Flag	Coverage Δ
unittests	`97.67% <99.66%> (+1.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

rozyczko

part 1 after a quick look

src/easydynamics/sample_model/sample_model.py

rozyczko · 2025-10-17T12:22:52Z

src/easydynamics/sample_model/sample_model.py

+Numeric = Union[float, int]
+
+
+class SampleModel(ObjBase, MutableMapping):


Shouldn't you inherit from TheoreticalModelBase or at least from BaseObjectCollection?

Probably? I'm not exactly sure

src/easydynamics/sample_model/sample_model.py

rozyczko · 2025-10-17T12:48:52Z

src/easydynamics/sample_model/sample_model.py

+            params = []
+        for comp in self.components.values():
+            params.extend(comp.get_parameters())
+        return params


or just

from itertools import chain # Create generator for temperature parameter temp_params = (self._temperature,) if self._temperature is not None else () # Create generator for component parameters comp_params = (param for comp in self.components.values() for param in comp.get_parameters()) # Chain them together and return as list return list(chain(temp_params, comp_params))

(slight teaching aspect for itertools - a VERY important python module)

rozyczko · 2025-10-17T12:51:56Z

src/easydynamics/sample_model/sample_model.py

+            if is_not_fixed and is_independent:
+                fit_parameters.append(parameter)
+
+        return fit_parameters


Again, not a bad loop (easy to see what's being done) but if you want to make it more "modular" you could use list comprehension, e.g.

def is_fit_parameter(param: Parameter) -> bool: """Check if a parameter can be used for fitting.""" return (not getattr(param, "fixed", False) and getattr(param, "_independent", True)) return [param for param in self.get_parameters() if is_fit_parameter(param)]

src/easydynamics/sample_model/sample_model.py

henrikjacobsenfys · 2025-10-20T11:07:30Z

@rozyczko Would you have another look when you have time? I'm working on other things the rest of the day and am away tomorrow, so no rush.

rozyczko

A few more comments after pondering on the file for too long ;)

rozyczko · 2025-10-31T12:47:41Z

src/easydynamics/sample_model/sample_model.py

+        """
+
+        CollectionBase.__init__(self, name=name)
+        TheoreticalModelBase.__init__(self, name=name)


This should probably be just

super().__init__(name=name)

to properly handle multiple inheritances. Otherwise it violates MRO best practices and makes maintenance more difficult (by being too explicit)

rozyczko · 2025-10-31T12:51:08Z

src/easydynamics/sample_model/sample_model.py

+        -------
+        List[ModelComponent]
+        """
+        return list(self)


The class mixes direct list access (list(self)) with a components property. Pick one approach:

Either make components the primary interface and remove list(self) calls

Or fully embrace the collection interface and remove the components property

Good point. I will embrace the collection

rozyczko · 2025-10-31T12:53:42Z

src/easydynamics/sample_model/sample_model.py

+            Name to assign to the component. If None, uses the component's own name.
+        """
+        if not isinstance(component, ModelComponent):
+            raise TypeError("component must be an instance of ModelComponent.")


Maybe uppercase Component since it is the start of the sentence?

rozyczko · 2025-10-31T12:54:20Z

src/easydynamics/sample_model/sample_model.py

+
+        # Add initial components if provided. Mostly used for serialization.
+        if data:
+            # Just to be safe


This comment adds no value. Either explain WHY it's needed or remove it, please.

rozyczko · 2025-10-31T12:58:52Z

src/easydynamics/sample_model/component_collection.py

+
+
+class SampleModel(CollectionBase, TheoreticalModelBase):
+    """


It would probably be useful to add __len__ so querying the model is simpler

>>> model = SampleModel() >>> model.add_component(Gaussian(name="g1")) >>> len(model)

To do this, just query super():

def __len__(self) -> int: return super().__len__()

Already works! :)

rozyczko · 2025-10-31T13:02:04Z

src/easydynamics/sample_model/sample_model.py

+        self.insert(index=len(self), value=component)
+
+    def remove_component(self, name: str):
+        """


Why find all indices if you only delete the first? This is either inefficient or you expect duplicate names?
Just delete the first index found.

rozyczko · 2025-10-31T13:04:00Z

src/easydynamics/sample_model/sample_model.py

+        List[ModelComponent]
+        """
+        return list(self)
+


Also, this creation of a new list every time components() is accessed is pretty wasteful. Either cache it or use list(self) directly as said earlier.

rozyczko · 2025-10-31T13:04:54Z

src/easydynamics/sample_model/component_collection.py

+        )  # noqa: E501
+
+    def convert_unit(self, unit: Union[str, sc.Unit]) -> None:
+        """


Please consider adding the rollback mechanism, which you did in ModelComponent.

rozyczko · 2025-10-31T13:05:56Z

src/easydynamics/sample_model/sample_model.py

+        if not self.components:
+            raise ValueError("No components in the model to evaluate.")
+        result = None
+        for component in list(self):
+            value = component.evaluate(x)
+            result = value if result is None else result + value


or just

if not self.components: raise ValueError("No components in the model to evaluate.") return sum(component.evaluate(x) for component in self)

rozyczko · 2025-10-31T13:09:35Z

src/easydynamics/sample_model/component_collection.py

+
+class SampleModel(CollectionBase, TheoreticalModelBase):
+    """
+    A model of the scattering from a sample, combining multiple model components.


Similarly, you could extend the API to include __contains__ so we can do:

>>> model = SampleModel() >>> gaussian = Gaussian(name="g1") >>> model.add_component(gaussian) >>> "g1" in model True >>> gaussian in model True >>> "nonexistent" in model False

with something like

def __contains__(self, item: Union[str, ModelComponent]) -> bool: if isinstance(item, str): # Check by component name return any(comp.name == item for comp in self) elif isinstance(item, ModelComponent): # Check by component instance return any(comp is item for comp in self) else: return False

henrikjacobsenfys · 2025-11-01T19:23:59Z

As always, very useful comments :) here's the updated version

damskii9992

First and by far the biggest comment.
Do you WANT your SampleModel to BE a list and dictionary? Or do you want it to contain a list/dictionary?
It should only be the former if you REALLY want to be able to iterate over your model, slice it to get components, or do len(sample_model) instead of, for example len(sample_model.components).
I don't really think you want it to be a list/dictionary . . .
And even if you did, you should probably write up your own collections class, as relying on CollectionBase is dangerous as it is subject to be removed (or at least heavily modified).

henrikjacobsenfys · 2025-11-12T15:13:49Z

First and by far the biggest comment. Do you WANT your SampleModel to BE a list and dictionary? Or do you want it to contain a list/dictionary? It should only be the former if you REALLY want to be able to iterate over your model, slice it to get components, or do len(sample_model) instead of, for example len(sample_model.components). I don't really think you want it to be a list/dictionary . . . And even if you did, you should probably write up your own collections class, as relying on CollectionBase is dangerous as it is subject to be removed (or at least heavily modified).

Well, I honestly don't care too much exactly what it is. I want to have a collection of components that I/the user can easily access and work with, I want it to be integrated with the rest of corelib, i.e. reuse as much of corelib as possible, which also helps minimizing the amount of code I have to write/maintain in dynamics-lib. CollectionBase checks all those boxes. I'm not attached to it in any way, though, so happy enough to do it in a different way.

Would you recommend the components to be stored as a dict or list?

henrikjacobsenfys · 2025-12-10T12:39:09Z

@damskii9992 Could I have a quick sanity check of the changes to ModelComponent and Gaussian to use ModelBase before I make the same changes to all the other components?

damskii9992 · 2025-12-10T13:43:40Z

src/easydynamics/sample_model/components/model_component.py

        self,
-        name="ModelComponent",
+        display_name="ModelComponent",
        unit: Optional[Union[str, sc.Unit]] = "meV",


Don't use Optional or Union for type hinting, from Python 3.10 and forward you simply write str | sc.Unit
Also, Optional essentially means it can also be a None, so it shouldn't even have been used here.

damskii9992 · 2025-12-10T13:46:49Z

src/easydynamics/sample_model/components/model_component.py

    def free_all_parameters(self):
        """Free all parameters in the model component."""
-        for p in self.get_parameters():
+        for p in self.get_all_parameters():


This and above should be get_fittable_parameters since you can't set dependent Parameters fixed attribute.

damskii9992 · 2025-12-10T13:48:49Z

src/easydynamics/sample_model/components/model_component.py


        old_unit = self._unit
-        pars = self.get_parameters()
+        pars = self.get_all_parameters()


Shouldn't this be get_all_variables so you also convert DescriptorNumbers? You had a Plancks constant descriptor somewhere right? Shouldn't that one also be converted?

No I don't think so, since Planck's constant can't be converted to e.g. microeV. So I'll leave it as it is for now, and figure out the units issue when I get to the DiffusionModel

damskii9992 · 2025-12-10T13:49:48Z

src/easydynamics/sample_model/components/model_component.py

    def __init__(
        self,
-        name="ModelComponent",
+        display_name="ModelComponent",


You can also remove the default display_name, it is not a mandatory attribute :)

Hmmm, but I like names... I'll probably remove it

henrikjacobsenfys · 2025-12-11T15:36:25Z

@damskii9992 @rozyczko all ready for another review when you have time :)

rozyczko

One important bug needs to be addressed.
The rest is for consideration

rozyczko · 2025-12-15T17:45:22Z

src/easydynamics/sample_model/component_collection.py

+        self,
+        display_name: str = "MyComponentCollection",
+        unit: str | sc.Unit = "meV",
+        components: List[ModelComponent] = [],


Oh my... Mutable Default Argument bug. Using [] as a default argument is a well-known Python anti-pattern.
Python’s default arguments are evaluated once when the function is defined, not each time the function is called
Use None and create the list inside the function:

components: List[ModelComponent] | None = None,

then

components = components or []

Check out this example:

def append_to(element, to=[]): to.append(element) return to

then call it twice with different args

my_list = append_to(12) print(my_list) my_other_list = append_to(42) print(my_other_list)

What did you expect in the second case? [42]? :)

What is this madness?!

rozyczko · 2025-12-15T17:57:10Z

src/easydynamics/sample_model/components/gaussian.py

    """
    Gaussian function: area/(width*sqrt(2pi)) * exp(-0.5*((x - center)/width)^2)
    If the center is not provided, it will be centered at 0 and fixed, which is typically what you want in QENS.



your component functions are missing get_all_variables Implementation, relying on the parent class implementation.
The parent ModelBase.get_all_variables() may not find _area, _center, _width since they're not passed to super().init() anymore.

I am indeed relying on the parent class implementation, but it works :)

Yes, the new ModelBase classs' get_all_variables method automatically finds the parameters of of the class if they are defined as properties which return a Parameter :)

rozyczko · 2025-12-15T17:58:55Z

src/easydynamics/sample_model/components/gaussian.py

+        super().__init__(
+            display_name=display_name,
+            unit=unit,
+        )


Gaussian calls super().__init__() first (which relies on self._unit), but _unit is set inside ModelComponent.__init__ via validate_unit. However, other components (Lorentzian, DHO) set self._unit = unit before calling super. This is inconsistent

Whoops, the Gaussian does it right, setting self._unit = unit before calling super is redundant. Will fix.

rozyczko · 2025-12-15T18:00:02Z

src/easydynamics/sample_model/components/polynomial.py

        )

-    def get_parameters(self) -> list[Parameter]:
+    def get_all_variables(self) -> list[DescriptorBase]:


This overrides get_all_variables properly but other components use get_all_parameters. The naming is inconsistent across the codebase

Yup, this is because of how the coefficients of the polynomial are created and stored. ModelBase does not look through lists of Parameters, so I have to do it manually. All the other get_all_ from ModelBase rely on get_all_variables, so this was the only one that needed to be overridden.

It may make more sense to update ModelBase to also look through lists to check for Parameters (and Descriptors)

rozyczko · 2025-12-15T18:02:04Z

src/easydynamics/sample_model/component_collection.py

+        Parameters
+        ----------
+        name : str
+            Name of the sample model.


Says name but param is display_name

Damn, thought I had gotten them all

rozyczko · 2025-12-15T18:03:31Z

src/easydynamics/sample_model/component_collection.py

@@ -0,0 +1,296 @@
+import warnings


I'm trying to consistently use the new style, but clearly I'm not good at consistency. Should be better now

rozyczko · 2025-12-15T18:04:32Z

tests/unit_tests/sample_model/test_component_collection.py

@@ -0,0 +1,372 @@
+from copy import copy


Given this is part of the EasyScience ecosystem which relies heavily on as_dict()/from_dict() serialization, there are no tests for serializing/deserializing ComponentCollection.

Good point. Copilot was happy to write those tests :)

damskii9992

I have not reviewed the unit tests yet, but I feel like there is plenty here to look at first.

damskii9992 · 2025-12-17T12:56:05Z

src/easydynamics/sample_model/components/model_component.py

-        name="ModelComponent",
-        unit: Optional[Union[str, sc.Unit]] = "meV",
-        **kwargs: Any,
+        display_name: str = None,


Typehint should be str | None

damskii9992 · 2025-12-17T12:57:16Z

src/easydynamics/sample_model/components/model_component.py

    ):
        self.validate_unit(unit)
-        super().__init__(name=name, **kwargs)
+        super().__init__(display_name=display_name)


No custom unique_name? Is that on purpose or did you just forget it?

damskii9992 · 2025-12-17T13:02:55Z

src/easydynamics/sample_model/components/polynomial.py

-        name: Optional[str] = "Polynomial",
-        coefficients: Optional[Sequence[Union[Numeric, Parameter]]] = (0.0,),
-        unit: Union[str, sc.Unit] = "meV",
+        display_name: str = "Polynomial",


Isn't this missing a typehint of None?

damskii9992 · 2025-12-17T13:05:18Z

src/easydynamics/sample_model/components/polynomial.py

+        unit: str | sc.Unit = "meV",
    ):
-        self.validate_unit(unit)
+        super().__init__(display_name=display_name, unit=unit)


Also (possibly) missing a custom unique_name? I won't write it on the rest of the places.

damskii9992 · 2025-12-17T13:07:27Z

src/easydynamics/sample_model/components/polynomial.py

+                param = Parameter(name=f"{display_name}_c{i}", value=float(coef))
            else:
                raise TypeError(
                    "Each coefficient must be either a numeric value or a Parameter."


You're allowing the user to pass Parameters? I thought we decided against that . . .

Well, I needed it to make serialisation work with the old base...

As discussed, I'll keep it for now, not use it unless I feel like I really need to, then remove it once SampleModel exists :)

damskii9992 · 2025-12-17T14:40:52Z

src/easydynamics/sample_model/component_collection.py

+        for component in self.components:
+            if hasattr(component, "area"):
+                area_params.append(component.area)
+                total_area += component.area.value


Don't you need to check their units? Remember that we can add Parameters together. You might want to use that here.

Good point! I'll do that

damskii9992 · 2025-12-17T14:46:18Z

src/easydynamics/sample_model/component_collection.py

+            raise ValueError("Total area is zero; cannot normalize.")
+
+        if not np.isfinite(total_area):
+            raise ValueError("Total area is not finite; cannot normalize.")


How would this ever happen? We should not allow Parameters to have infinite values and then this could never be infinite.

I think this also accounts for NaN's. Either way I think it's fine to have a check even if it will never be used if things go as we expect :)

Unnecessary checks does make code slower, but only by a little. Just wanted to point it out in case you hadn't realized ^^

damskii9992 · 2025-12-17T14:57:10Z

src/easydynamics/sample_model/component_collection.py

+
+        if not self.components:
+            raise ValueError("No components in the model to evaluate.")
+        return sum(component.evaluate(x) for component in self.components)


Does this even work? You are trying to sum the contributions element-wise, right? So if x is an array, you would be trying to use sum on a tuple of arrays/lists.

#56 (comment)

damskii9992 · 2025-12-17T15:01:07Z

src/easydynamics/sample_model/component_collection.py

+        Fix all free parameters in the model.
+        """
+        for param in self.get_all_parameters():
+            param.fixed = True


As previously mentioned, this and below should be get_fittable_parameters as you can't set the fixed attribute on dependent Parameters.

damskii9992 · 2025-12-17T15:02:11Z

src/easydynamics/sample_model/component_collection.py

+
+        if isinstance(item, str):
+            # Check by component name
+            return any(comp.display_name == item for comp in self.components)


In all of this class, you should really think about where you use display_name and where you use unique_name.
display_name is ONLY meant for displaying, any and all logic should be using the unique_name.

henrikjacobsenfys added 4 commits October 14, 2025 19:54

Add basic sample_model and tests

d1c20a4

Clean up, fix tests

a782693

Improve tests

2fb844b

Update example and tests

d4309fc

henrikjacobsenfys added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] high Should be prioritized soon labels Oct 17, 2025

henrikjacobsenfys added this to the First release milestone Oct 17, 2025

henrikjacobsenfys changed the title A Sample Model Oct 17, 2025

henrikjacobsenfys added 4 commits October 17, 2025 11:14

Add one more test

cde9b4b

Added a few tests

6f797f8

a few more tests

3435da5

fix a test

c0a4f2c

rozyczko requested changes Oct 17, 2025

View reviewed changes

henrikjacobsenfys added 2 commits October 17, 2025 20:33

respond to PR comments

a4ba794

Use CollectionBase

d1d28d2

henrikjacobsenfys added 4 commits October 20, 2025 13:08

Update example

ce49f05

Merge branch 'develop' into SampleModel2

0eb1bba

Update to new ModelComponent

07c070a

Cleanup and a few tests

3a7187c

rozyczko reviewed Oct 31, 2025

View reviewed changes

Respond to reviewer comments

8aa43c0

Tests :)

28d0b0e

henrikjacobsenfys requested a review from damskii9992 November 12, 2025 12:23

damskii9992 reviewed Nov 12, 2025

View reviewed changes

Get rid of CollectionBase

a41277b

henrikjacobsenfys requested review from damskii9992 and rozyczko November 13, 2025 16:02

henrikjacobsenfys added 2 commits November 13, 2025 17:09

Remove outcommented code

8d3ad4d

update docstring

0d3e3eb

henrikjacobsenfys marked this pull request as draft November 21, 2025 13:42

henrikjacobsenfys added 2 commits December 10, 2025 11:32

Update model_component to NewBase

4543ef7

Update model_component and tests

844d414

damskii9992 reviewed Dec 10, 2025

View reviewed changes

henrikjacobsenfys added 9 commits December 11, 2025 09:43

update model_component

c155a7d

Update gaussian

bde91b5

update Lorentzian

fa50c93

update DHO

ce8df0e

update delta function

6ade763

Update voigt

da2462c

Update Polynomial

6d5a6af

Small updates to component docstrings

6ccd646

Update sample_model

cdd208b

henrikjacobsenfys requested a review from damskii9992 December 11, 2025 19:45

henrikjacobsenfys added 2 commits December 12, 2025 05:57

rename to ComponentCollection

7a5dd45

Add a few tests, simplify polynomial

4f7608e

henrikjacobsenfys marked this pull request as ready for review December 12, 2025 05:11

Update examples

2d9c134

henrikjacobsenfys changed the title ~~Sample Model~~ ComponentCollection Dec 12, 2025

rozyczko requested changes Dec 15, 2025

View reviewed changes

henrikjacobsenfys added 3 commits December 15, 2025 20:09

Update based on PR comments

65a941a

Update type hints

f084461

Add tests of to and from dict

23c8660

damskii9992 requested changes Dec 17, 2025

View reviewed changes

		Numeric = Union[float, int]


		class SampleModel(ObjBase, MutableMapping):

ComponentCollection #56

Are you sure you want to change the base?

ComponentCollection #56

Conversation

henrikjacobsenfys commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henrikjacobsenfys commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rozyczko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henrikjacobsenfys commented Oct 20, 2025

Uh oh!

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

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!

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!

henrikjacobsenfys commented Nov 1, 2025

Uh oh!

damskii9992 left a comment

Choose a reason for hiding this comment

Uh oh!

henrikjacobsenfys commented Nov 12, 2025

Uh oh!

henrikjacobsenfys commented Dec 10, 2025

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!

henrikjacobsenfys commented Oct 17, 2025 •

edited

Loading

henrikjacobsenfys commented Oct 17, 2025 •

edited

Loading

codecov bot commented Oct 17, 2025 •

edited

Loading

rozyczko Dec 15, 2025 •

edited

Loading