ADR Suggestion New EasyScience base classes.

[damskii9992]

General

As has been outlined in detail in #110, the current base classes ObjBase, BasedBase and CollectionBase are over-engineered and convoluted classes which are difficult to edit and maintain.
This is mostly caused by two key features: "Parameter injection", which allows dynamic classes with attributes defined at instantiation, and "interface switching" which allows on-the-fly changing of the interface between different third-party calculators without loss of the current state.

Both of these features are undesirable for different reasons. Parameter injection is undesirable because they encourage creation of classes with unknown or changing API's, making it extremely difficult to develop and maintain features using these classes.
Interface switching is undesirable due to improper separation of responsibilities and very confusing logic, as outlined in #160.

This ADR has an initial implementation which can be reviewed in #159

Current Implementation

The current implementation has already been outlined in #110

Proposed Implementation

I propose to remove the 3 base classes ObjBase, BasedBase and CollectionBase in the future, and replace them with new simpler classes. For now, the new base classes will coexist with the old base classes until all dependent libraries have switched.

Specifically I propose 2 new base classes: NewBase which is the new root-base class of EasyScience, replacing BasedBase and ModelBase which inherits from NewBase but adds functionality to accommodate Parameter class attributes.

NewBase

This class will be renamed in the future, when we have properly switched to the new classes.
NewBase inherits from no other class, unlike BasedBase which inherits from SerializerComponent to add serialization capabilities. Instead NewBase implements those serialization capabilities itself directly, avoiding the need for an extra inheritance layer. Specifically it adds 3 methods:

def _arg_spec(self) -> Set[str]:
    spec = getfullargspec(self.__class__.__init__)
    names = set(spec.args[1:])
    return names

Which is needed for the SerializerBase class to serialize the NewBase class. The SerializerBase (which should be renamed), is a class with static methods which holds logic for generic serialization/deserialization. In EasyScience, serialization is handed by the as_dict method:

def as_dict(self, skip=None) -> Dict[str, Any]:
return SerializerBase._convert_to_dict(self, skip=skip)

Which serializes the class to a dictionary. For serialization to JSON or other formats, this dictionary can then be re-serialized. Deserialization is handled by the from_dict method, which by default uses the generic deserialization of the SerializerBase class. Inherited classes should then overwrite this method for custom deserialization.

@classmethod
def from_dict(cls, obj_dict) -> None:
    if not SerializerBase._is_serialized_easyscience_object(obj_dict):
        raise ValueError('Input must be a dictionary representing an EasyScience object.')
    kwargs = SerializerBase.deserialize_dict(obj_dict)
    return cls(**kwargs)

Additionally, the NewBase class implements the unique_name attribute and the logic to the global_object map, as well as the optional display_name attribute.

Finally the class implements the dunder methods:

• __dir__
• __copy__
• __deepcopy__
• __repr__

ModelBase

This class inherits from NewBase and adds functionality for Parameter attributes which are modifiable for the user but not replaceable. This is done by implementing the setter method to simply overwrite the value of the Parameter whereas the getter method return the Parameter instance, allowing users direct access to the Parameters methods and attributes, like fixed or min, i.e.:

@property
def my_param(self) -> Parameter:
    return self._my_param

@my_param.setter
def my_param(self, new_value: float) -> None:
    self._my_param.value = new_value

When Parameter attributes are defined as such, ModelBase provides functionality for correct deserialization by overwriting the from_dict method to set the underlying _my_param attributes, since the class does not (and should not) take Parameter objects as inputs in its constructor.

Additionally ModelBase provides 3 convenience method to get all its Parameters. get_all_parameters, which recursively finds all DescriptorNumber or Parameter attributes in the class:

def get_all_parameters(self) -> List[DescriptorNumber]:
    params = []
    for attr_name in dir(self):
        attr = getattr(self, attr_name)
        if isinstance(attr, DescriptorNumber):
            params.append(attr)
        elif hasattr(attr, 'get_all_parameters'):
            params += attr.get_all_parameters()
    return params

And also get_fit_parameters which filters out dependent Parameters and DescriptorNumbers to only provide fittable Parameters and get_free_parameters which additionally filters out all fixed Parameters.

SerializerBase

The SerializerBase class is the current class responsible for serialization/deserialization of objects. This class gets extended with a deserialize_dict static method, which takes a dictionary and deserializes it, using classes own from_dict method if they're EasyScience classes, and the generic deserializer otherwise.

This method allows classes themselves to define a from_dict method to determine how they are deserialized, instead of just assuming that the __init__ constructor works with a serialized dictionary, which it won´t for Parameter attributes.

[henrikjacobsenfys]

Disclaimer: I haven't spent much time studying our base classes. This looks mostly good to me, but with some questions/comments.

I'm not sure if I like that the get_all_parameters method also gets DescriptorNumbers. The only DescriporNumbers I use so far are for unit handling, and I don't want hbar to show up when asking for a list of Parameters. If you want to include DescriptorNumbers we may need a fourth get_parameters type method?

I find it convenient, perhaps almost required, to pass Parameters as inputs to constructors. I could of course be wrong, but I'm using it in my Convolution class. Specifically, the offset, which is a fittable Parameter.

May be worth it to add display_name here - I see that it's in the PR :)

[damskii9992]

The display_name is mentioned here ;) But Piotr wants it moved to the ModelBase class, what do you think about that?

The get_all_parameters getting also DescriptorNumbers was based on the implementation in EasyDiffraction, maybe @AndrewSazonov has an opinion on that? We could have a fourth method which doesn't include DesriptorNumbers but filters out both dependent parameters and fixed parameters.

For passing Parameters to class constructors, using Piotrs words, that screams for a refactoring ;) Essentially Parameters should only live within their own classes. If they are accessed in other classes, then you pass the class which they belong to, to that other class. Like how the Analysis class doesn't take the Parameters from a SampleModel as input, but rather the entire SampleModel class.

[henrikjacobsenfys]

Reading is apparently hard for me haha. I have no strong opinion on which base class the name lives in.

I'll think about the refactoring!

[damskii9992]

Hey Henrik.
I discussed your point about get_all_parameters with Piotr, and he agrees. So we came to the conclusion to make a fourth method called get_all_variables which gets all Descriptors (because this is still useful) and change get_all_parameters to only yield Parameters.
Would this work for you?
We call get get_all_variables, because the module which Parameters and Descriptors live under are called variables :)

[seventil]

Really apreciate the changes that improve the definition of classes interface.
Although it is mentioned that the current implementation allows for on-the-fly change of interfaces for calculators, the proposed implementation will have that as well with the proper adaptors (as we discussed earlier)

[damskii9992]

Yup, once we implement the idea originally discussed in #160

[damskii9992]

Following discussions in-person. It has been decided to rename as_dict, which was the serialization method name used in the old base classes, to to_dict, because it is an action of serializing the object to a dictionary and it aligns more closely with the de-serialization method name of from_dict.

It was also discussed and agreed to change get_all_parameters to only yield Parameters and not Descriptors and then add a fourth method called get_all_variables which will get both all Descriptors (string, number etc.) and Parameters.

It was also decided to rename get_fit_parameters to get_fittable_parameters, but due to this breaking minimizations, an alias will be provided for now until we have fully moved to the new base classes.

[henrikjacobsenfys]

Doh. I'd written my own get_fit_parameters at some point and confused it with the corelib one. Just ignore me

[henrikjacobsenfys]

Wait, but this is what's in based_base right now (in develop). So it does do what I thought it did, no? Not that it really matters, we completely agree on what the new base class should do in this regard

def get_fit_parameters(self) -> List[Parameter]:
        """
        Get all objects which can be fitted (and are not fixed) as a list.

        :return: List of `Parameter` objects which can be used in fitting.
        """
        fit_list = []
        for key, item in self._kwargs.items():
            if hasattr(item, 'get_fit_parameters'):
                fit_list = [*fit_list, *item.get_fit_parameters()]
            elif isinstance(item, Parameter):
                if item.independent and not item.fixed:
                    fit_list.append(item)
        return fit_list

[damskii9992]

:O You're right! Good catch! I must've missed it. So the old get_fit_parameters is indeed the new get_free_parameters. I should make sure our alias points at the right method then. Thanks for noticing :)

[damskii9992]

This is why we need to rename the methods. Clearly their names were too confusing before XD

[henrikjacobsenfys]

I'm missing an upside-down smile reaction

[rozyczko]

Let's also address __reduce__ - do we see pickling EasyObjects an important feature?

One potential use case is if we want to pass an EasyObject to another process in the MPI world. It's probably easier to rely on built-in pickling that to serialize it into a dict in this case.

[damskii9992]

That is indeed a valid use-case for a __reduce__ method, but we can simply add that when we add MPI processing then, and even then only if we end up actually using it.
I don't personally see any current reason to implement it.

[rozyczko]

We need explicit migration plans.

• Keep new base classes alongside old ones for full backward compatibility
• Have new libraries/apps only use the new base classes
• Set timeframe for existing libraries/apps to refit the design (currently only reflectometry and crystallography)

[damskii9992]

I will personally work on EasyCrystallography's migration since it will be necessary for designing my SampleModel class. I think we need to give ample time for migration, so would a 6-month deadline be appropriate?
Do you envision that we keep the old base classes perpetually or just for a year or something?

[rozyczko]

I would like to have a deadline, so we can plan our projects accordingly.
6 months would be June 2026, and by then I expect all the libraries (and most apps) be in at least at the MVP stage.
Let's then aim at that.

[rozyczko]

This class inherits from NewBase and adds functionality for Parameter attributes which are modifiable for the user but not replaceable

Preventing replacing a Parameter object (only changing its .value) might be surprising. What if a user legitimately wants to swap in a different Parameter instance? Say, reset to default, testing/mocking,
Or a case like: you run initial fit, examin results, and now want to refine the bounds by replacing the parameter with new min/max.

Maybe we need replace_parameter?

my_model.p = 1.0 # updates value only
new_p = Parameter(name="p", value=1.0, min=0.5, max=2.0)
my_model.replace_parameter("p", new_p)

[damskii9992]

Allowing users to replace parameters risk breaking the model. We could of course have checks on those replacements, but I thought we also wanted to avoid having users creating Parameters directly?
For resetting to default you can simply have a reset() method on the model class which sets the values back to default, not that I see that being particularly useful . . .
For testing/mocking it is only us developers and we can just edit the internal hidden attribute.

[rozyczko]

Serialization - currently the idea is to use the JSON format. Maybe it would be beneficial to support a plug-in registry for (future) custom deserializers for known external types? LIke CIF (used by imaging, diffraction). It would probably be better to have them in the base class but not necessarily implement now, just make a mechanism with which they can be easily added.

However, the registry itself could already be added.

[rozyczko]

The consensus seems to be to:

• have the basic JSON (de)serializer in the core, since it affects all the libraries/techniques
• have technique specific (de)serializers in the corresponding library code (e.g. ORSO format in EasyReflectometryLib)
• have multiple technique specific (de)serializers in a separate utility module (e.g. CIF format for imaging and diffraction in EasyCrystallography)

[damskii9992]

We should probably make an ADR on this at the organization-level. Including how we first serialize to dict, and then from that re-serialize to other formats.