Sample and resolution model #69

henrikjacobsenfys · 2026-01-13T09:43:36Z

First draft of sample and resolution model. If I could get some feedback on the general outline that'd be great. There's still a lot of polishing and testing to be done, but the general idea is there. Try out the sample_model.ipynb notebook in examples :)

codecov · 2026-01-13T09:45:21Z

Codecov Report

❌ Patch coverage is 92.11470% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.26%. Comparing base (2ef5011) to head (ed99238).

Files with missing lines	Patch %	Lines
src/easydynamics/sample_model/sample_model.py	82.35%	21 Missing ⚠️
...iffusion_model/brownian_translational_diffusion.py	92.85%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #69      +/-   ##
===========================================
- Coverage    97.83%   96.26%   -1.58%     
===========================================
  Files           16       21       +5     
  Lines          786     1043     +257     
===========================================
+ Hits           769     1004     +235     
- Misses          17       39      +22

Flag	Coverage Δ
unittests	`96.26% <92.11%> (-1.58%)`	⬇️

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

A few serious issues highlighted in the code. I'll look at minor issues once these are addressed.

Additionally, I think that the naming and inheritance structure is confusing:

SampleModelBase → SampleModel, ResolutionModel, BackgroundModel
But ResolutionModel and BackgroundModel are not sample models - they're instrument-related
InstrumentModel inherits from NewBase (not ModelBase), creating inconsistency

Recommendation: Consider renaming:

SampleModelBase → QDependentModelBase or SpectralModelBase
Or create separate base classes for InstrumentComponentModelBase vs SampleModelBase

rozyczko · 2026-01-15T13:35:51Z

src/easydynamics/sample_model/sample_model_base.py

+from easyscience.base_classes.model_base import ModelBase
+from numpy.typing import ArrayLike
+
+from easydynamics.sample_model import ComponentCollection


package-level import imports all components and diffusion models.
This makes the base class heavy and increases the chance of circular imports.
Consider direct import from the local module (ComponentCollection) to keep the base layer minimal and avoid side effects.

rozyczko · 2026-01-15T13:37:14Z

src/easydynamics/sample_model/model_base.py

+        components: ModelComponent | ComponentCollection | None = None,
+        Q: Q_type | None = None,


Responsibility split is blurry: SampleModelBase owns ComponentCollection and Q, while SampleModel also owns diffusion models and temperature. There is duplication of evaluation logic. Consider a clearer separation where base handles only collection evaluation and subclasses inject preprocessing (like detailed balance) .

rozyczko · 2026-01-15T13:43:34Z

src/easydynamics/sample_model/model_base.py

+    # --------------------------------------------------------------------
+
+    def __repr__(self):
+        return f"{self.__class__.__name__}(unique_name={self.unique_name}, unit={self.unit}), Q = {self.Q}, components = {self.components}"


Didn't you mean self._unit? There's no self.unit here. A test would catch it

rozyczko · 2026-01-15T13:44:39Z

src/easydynamics/sample_model/resolution_model.py

+    def append_component(self, component: ModelComponent):
+        """Append a component to the ResolutionModel. Does not allow DeltaFunction or Polynomial components, as these are not physical resolution components.


This blocks disallowed components, but append_components_from_collection() and the components setter in the base class bypass that check, allowing forbidden components to be inserted indirectly.

I think I've fixed this. Tests will show :)

rozyczko · 2026-01-15T13:48:14Z

src/easydynamics/sample_model/sample_model_base.py

+    def _validate_and_convert_Q(self, Q: Q_type) -> np.ndarray:
+        """


Consider accepting sc.Variable for consistency

Good idea. Made a todo about moving entirely to scipp for internal use of Q. Will be a while though

rozyczko · 2026-01-15T13:50:07Z

src/easydynamics/sample_model/component_collection.py

+    def append_component(self, component: ModelComponent) -> None:
        if not isinstance(component, ModelComponent):


Changing Q, adding/removing components, or altering diffusion models does not invalidate or regenerate _component_collections. This can silently return stale results. Consider lazy regeneration or a dirty-flag pattern on Q, components, and diffusion models, as mentioned by you earlier in the discussion

Yup, this will come last once the rest is polished. It's not too terrible to ask users to run generate_component_collections() before they start doing stuff..

rozyczko · 2026-01-15T13:54:54Z

src/easydynamics/sample_model/sample_model_base.py

+            raise TypeError(
+                f"unit must be None, a string, or a scipp Unit, got {type(unit).__name__}"
+            )
+        self._unit = unit


Missing unit property - should be in the base class

rozyczko · 2026-01-15T13:56:15Z

src/easydynamics/sample_model/model_base.py

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


SampleModelBase.__init__ never initializes self._component_collections but evaluate accesses it! This will raise AttributeError

henrikjacobsenfys · 2026-01-16T14:41:44Z

It's missing a few test that I will do soon, and also the "dirty" pattern, but everything else should be there @rozyczko
I know the weekend just started so no rush, just feels good to send it off :)

henrikjacobsenfys added 6 commits January 12, 2026 11:01

Create the initial files

f44832a

A small beginning

7202b9e

Update first draft of sample_model

bf3dffd

added lots of properties, almost working minimal example

bb30030

Update example

2c5d09c

Small updates

680f15f

henrikjacobsenfys assigned rozyczko Jan 13, 2026

henrikjacobsenfys added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority labels Jan 13, 2026

henrikjacobsenfys added 3 commits January 14, 2026 08:25

Make resolution and background model

7e86631

Fix tests

67296fa

remove instrument_model

d7e4b30

rozyczko requested changes Jan 15, 2026

View reviewed changes

henrikjacobsenfys added 8 commits January 15, 2026 19:44

Improve append_component

34f63f2

refactoring, simplifications

46fb4d4

Start testing

ee2f980

finish base class test?

8f37537

test utils, 100% model_base

274a13e

test resolution_model

a6d297d

test background model

3e78acc

Almost all tests

6fd310a

henrikjacobsenfys requested a review from rozyczko January 16, 2026 14:37

henrikjacobsenfys added 2 commits January 16, 2026 15:50

Update example

4d41475

fix example

ed99238

		components: ModelComponent \| ComponentCollection \| None = None,
		Q: Q_type \| None = None,

		def append_component(self, component: ModelComponent):
		"""Append a component to the ResolutionModel. Does not allow DeltaFunction or Polynomial components, as these are not physical resolution components.

		def _validate_and_convert_Q(self, Q: Q_type) -> np.ndarray:
		"""

		def append_component(self, component: ModelComponent) -> None:
		if not isinstance(component, ModelComponent):

Sample and resolution model #69

Are you sure you want to change the base?

Sample and resolution model #69

Uh oh!

Conversation

henrikjacobsenfys commented Jan 13, 2026

Uh oh!

codecov bot commented Jan 13, 2026 • 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!

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 Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 13, 2026 •

edited

Loading