Create new easyscience base class and model_base class. #159

damskii9992 · 2025-11-19T12:21:21Z

No description provided.

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['[scope] bug', '[scope] enhancement', '[scope] documentation', '[scope] significant', '[scope] maintenance']

Co-authored-by: Christian Dam Vedel <158568093+damskii9992@users.noreply.github.com>

rozyczko

No dynamic parameter injections!
I think this is a start of something awesome.
Next stop - the interface or rather, the calculator factory...

rozyczko · 2025-11-20T09:35:00Z

src/easyscience/base_classes/new_base.py

+        This method is used by the serializer to determine which arguments are needed
+        by the constructor to deserialize the object.
+        """
+        base_cls = getattr(self, '__old_class__', self.__class__)


__old_class__ is a part of the old design, set up in addLoggedProp which I think you wanted to avoid?

How about

spec = getfullargspec(self.__class__.__init__)

NewBase doesn't use dynamic class creation and the __old_class__ attribute is never set on NewBase instances...

rozyczko · 2025-11-20T09:40:58Z

src/easyscience/base_classes/new_base.py

+    It provides serialization capabilities as well as unique naming and display naming.
+"""
+
+    def __init__(self, unique_name: Optional[str] = None, display_name:  Optional[str] = None):


I think NewBase should only deal with unique_names. Why do you consider display_name to be a part of it? It seems too unimportant - can be set by classes which inherit NewBase...

This is why we should probably have an ADR discussion. To discuss what goes into the ground base class (this would be NewBase) and what goes into the derived classes :)
But I'm not against moving it into ModelBase.

src/easyscience/base_classes/new_base.py

rozyczko · 2025-11-20T09:44:24Z

src/easyscience/io/serializer_base.py

+                else:
+                    out_dict[key] = SerializerBase._convert_from_dict(value)
+        return out_dict
+


This implementation smells. Let's refactor it so it is easier to test.

rozyczko · 2025-11-20T09:47:17Z

src/easyscience/io/serializer_base.py

+    @staticmethod
+    def _deserialize_dict(in_dict: Dict[str, Any]) -> None:
+        """
+        Deserialize a dictionary using from_dict for EasyScience objects and SerializerBase otherwise.
+        :param in_dict: dictionary to deserialize
+        :return: deserialized dictionary
+        """
+        out_dict = {}
+        for key, value in in_dict.items():
+            if not key.startswith('@'):
+                if isinstance(value, dict) and "@module" in value and value["@module"].startswith("easy") and '@class' in value:  # noqa: E501
+                    module_name = value['@module']
+                    class_name = value['@class']
+                    try:
+                        module = __import__(module_name, globals(), locals(), [class_name], 0)
+                    except ImportError as e:
+                        raise ImportError(f'Could not import module {module_name}') from e
+                    if hasattr(module, class_name):
+                        cls_ = getattr(module, class_name)
+                        if hasattr(cls_, 'from_dict'):
+                            out_dict[key] = cls_.from_dict(value)
+                        else:
+                            out_dict[key] = SerializerBase._convert_from_dict(value)
+                    else:
+                        raise ValueError(f'Class {class_name} not found in module {module_name}.')
+                else:
+                    out_dict[key] = SerializerBase._convert_from_dict(value)
+        return out_dict
+


@staticmethod def _deserialize_dict(in_dict: Dict[str, Any]) -> Dict[str, Any]: """ Deserialize a dictionary using from_dict for ES objects and SerializerBase otherwise. This method processes constructor arguments, skipping metadata keys starting with '@'. :param in_dict: dictionary to deserialize :return: deserialized dictionary with constructor arguments """ d = { key: SerializerBase._deserialize_value(value) for key, value in in_dict.items() if not key.startswith('@') } return d @staticmethod def _deserialize_value(value: Any) -> Any: """ Deserialize a single value, using specialized handling for ES objects. :param value: :return: deserialized value """ if not SerializerBase._is_serialized_easyscience_object(value): return SerializerBase._convert_from_dict(value) module_name = value['@module'] class_name = value['@class'] try: cls = SerializerBase._import_class(module_name, class_name) # Prefer from_dict() method for ES objects if hasattr(cls, 'from_dict'): return cls.from_dict(value) else: return SerializerBase._convert_from_dict(value) except (ImportError, ValueError) as e: # Fallback to generic deserialization if class-specific fails return SerializerBase._convert_from_dict(value) @staticmethod def _is_serialized_easyscience_object(value: Any) -> bool: """ Check if a value represents a serialized ES object. :param value: :return: True if this is a serialized ES object """ return ( isinstance(value, dict) and "@module" in value and value["@module"].startswith("easy") and '@class' in value ) @staticmethod def _import_class(module_name: str, class_name: str): """ Import a class from a module name and class name. :param module_name: name of the module :param class_name: name of the class :return: the imported class :raises ImportError: if module cannot be imported :raises ValueError: if class is not found in module """ try: module = __import__(module_name, globals(), locals(), [class_name], 0) except ImportError as e: raise ImportError(f'Could not import module {module_name}') from e if not hasattr(module, class_name): raise ValueError(f'Class {class_name} not found in module {module_name}.') return getattr(module, class_name)

Agreed, this is much better. I took inspiration from the _convert_from_dict method, which is why it ended up not being properly factorised.
The _is_serialized_easyscience_object method might be taking it too far though, being a single logical check.

rozyczko · 2025-11-20T09:51:08Z

src/easyscience/base_classes/model_base.py

+
+class ModelBase(NewBase):
+    """
+    This is the base class for all model classes in EasyScience.


ModelBase is where we should add display_name, I think?

src/easyscience/base_classes/model_base.py

rozyczko · 2025-11-20T09:57:03Z

src/easyscience/base_classes/model_base.py

+        params = []
+        for attr_name in dir(self):
+            attr = getattr(self, attr_name)


return [param for param in self.get_fit_parameters() if param.fixed]

Yeah, I considered this approach too. It's much simpler, for the other method too. The reason I didn't choose it was for performance. This implementation only runs through the list of attributes once, your suggestion runs through the produced list again to re-filter it. The performance gain is probably so minimal it isn't worth it though :/

rozyczko · 2025-11-20T09:57:20Z

src/easyscience/base_classes/model_base.py

+    def get_free_parameters(self) -> List[Parameter]:
+        """
+        Get all parameters which are currently free to be fitted as a list.
+
+        :return: List of `Parameter` objects.
+        """
+        params = []
+        for attr_name in dir(self):
+            attr = getattr(self, attr_name)
+            if isinstance(attr, Parameter) and not attr.fixed and attr.independent:
+                params.append(attr)
+        return params


return [param for param in self.get_fit_parameters() if not param.fixed]

rozyczko · 2025-11-20T09:59:30Z

src/easyscience/base_classes/model_base.py

+
+    @classmethod
+    def from_dict(cls, obj_dict: Dict[str, Any]) -> None:
+        """


multiple conditionals are icky. I'd like to rewrite this to be more modular, but can't be arsed now.

I fixed it. Using the _is_serialized_easyscience_object method you refactored in the SerializerBase class. Clearly it was useful to refactor it even though it was only a simple logical check.

codecov · 2025-11-20T14:17:11Z

Codecov Report

❌ Patch coverage is 88.02589% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.82%. Comparing base (a3a9bcb) to head (f8b0817).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines	Patch %	Lines
...yscience/variable/parameter_dependency_resolver.py	63.41%	25 Missing and 5 partials ⚠️
src/easyscience/variable/parameter.py	92.45%	2 Missing and 2 partials ⚠️
src/easyscience/io/serializer_base.py	93.93%	1 Missing and 1 partial ⚠️
src/easyscience/variable/descriptor_number.py	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #159      +/-   ##
===========================================
+ Coverage    73.06%   80.82%   +7.76%     
===========================================
  Files           47       50       +3     
  Lines         3943     4246     +303     
  Branches       671      739      +68     
===========================================
+ Hits          2881     3432     +551     
+ Misses         868      629     -239     
+ Partials       194      185       -9

Flag	Coverage Δ
unittests	`80.82% <88.02%> (?)`

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

Files with missing lines	Coverage Δ
src/easyscience/base_classes/__init__.py	`100.00% <100.00%> (ø)`
src/easyscience/base_classes/based_base.py	`81.65% <100.00%> (+1.45%)`	⬆️
src/easyscience/base_classes/model_base.py	`100.00% <100.00%> (ø)`
src/easyscience/base_classes/new_base.py	`100.00% <100.00%> (ø)`
src/easyscience/global_object/map.py	`89.41% <100.00%> (+37.31%)`	⬆️
src/easyscience/variable/descriptor_number.py	`89.57% <88.88%> (+0.65%)`	⬆️
src/easyscience/io/serializer_base.py	`84.53% <93.93%> (+30.49%)`	⬆️
src/easyscience/variable/parameter.py	`90.49% <92.45%> (+0.13%)`	⬆️
...yscience/variable/parameter_dependency_resolver.py	`63.41% <63.41%> (ø)`

... and 4 files with indirect coverage changes

…ject` method

henrikjacobsenfys · 2025-11-20T19:28:40Z

src/easyscience/base_classes/new_base.py

+
+    @unique_name.setter
+    def unique_name(self, new_unique_name: str):
+        """Set a new unique name for the object. The old name is still kept in the map.


Why is the old name still kept in the map? Just curious; I haven't taken the time to even try to understand how the map works.

This was decided back in #16 under Andrews comment and Andreas reply. Thank god that we have ADR's and paper trails to follow now to remember these things ;)

henrikjacobsenfys · 2025-11-20T19:34:11Z

src/easyscience/base_classes/model_base.py

+                params += attr.get_all_parameters()
+        return params
+
+    def get_fit_parameters(self) -> List[Parameter]:


I'm pretty sure the fitter uses this method to choose which Parameters to fit, so we need to be a bit careful here

That is exactly why I named this method like this. I wanted to name it get_fittable_parameters, but the Fitter expects this method name :/
I will obviously make unit tests and integration tests to check that this implementation works with the Fitter :)

…cience into new_base_class

…ject` method

… into new_base_class

…erialization tests

Create new easyscience base class and model_base class.

a0aadea

github-actions bot requested changes Nov 19, 2025

View reviewed changes

damskii9992 added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] highest Urgent. Needs attention ASAP [area] base classes Changes to or creation of new base classes labels Nov 19, 2025

github-actions bot approved these changes Nov 19, 2025

View reviewed changes

rozyczko and others added 2 commits November 19, 2025 14:34

Constraints serialization (#148)

952da19

Co-authored-by: Christian Dam Vedel <158568093+damskii9992@users.noreply.github.com>

Interface factory unit tests (#151)

b5169f5

rozyczko reviewed Nov 20, 2025

View reviewed changes

Pr comments

66a199e

Simplify NewBase from_dict class using `_is_serialized_easyscience_ob…

690ce16

…ject` method

henrikjacobsenfys reviewed Nov 20, 2025

View reviewed changes

damskii9992 and others added 15 commits November 21, 2025 12:54

Sort imports

6687c33

Merge branch 'new_base_class' of https://github.com/EasyScience/EasyS…

5def3e7

…cience into new_base_class

Test new serializer_base methods except deserialize_dict

884df73

Documentation (#152)

2ba98e7

Last new SerializerBase test

0f8b932

Create new easyscience base class and model_base class.

8e3ab34

Sort imports

51100ad

Pr comments

4189a27

Simplify NewBase from_dict class using `_is_serialized_easyscience_ob…

64f09c1

…ject` method

Test new serializer_base methods except deserialize_dict

4e26dfc

Last new SerializerBase test

f396c70

Merge branch 'new_base_class' of https://github.com/easyscience/corelib…

ef6ee5c

… into new_base_class

Add as_dict tests

d19f256

Format and Lint

cecc7b0

Pixi update and final NewBase tests

fef80c0

damskii9992 added 6 commits November 24, 2025 23:17

First ModelBase tests

390ad80

Changes from ADR discussions

113ec7c

Test all get_variable methods

e10a4fa

Fix to recursive encoder to work with new base classes and nested des…

99c83e3

…erialization tests

Final ModelBase unit tests

5e67822

Add minimization integration test with ModelBase

f8b0817

damskii9992 closed this Nov 26, 2025

damskii9992 mentioned this pull request Nov 26, 2025

New base class #163

Open

Create new easyscience base class and model_base class. #159

Create new easyscience base class and model_base class. #159

Uh oh!

Conversation

damskii9992 commented Nov 19, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

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!

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!

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!

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

Uh oh!

Codecov Report

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 20, 2025 •

edited

Loading