DescriptorBase now is a NewBase class - #306
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## easylist-on-multifitter #306 +/- ##
===========================================================
+ Coverage 83.66% 83.81% +0.14%
===========================================================
Files 68 68
Lines 5271 5332 +61
===========================================================
+ Hits 4410 4469 +59
- Misses 861 863 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
damskii9992
left a comment
There was a problem hiding this comment.
More work to do. You forgot the biggest part of the change, the removal of the name attribute.
| ``DescriptorBase`` is a ``NewBase`` object. As such every descriptor | ||
| is registered in the global object map under its ``unique_name``, | ||
| has an optional ``display_name`` and is serialized with | ||
| ``to_dict``/``from_dict``. Descriptors and parameters can | ||
| therefore be held directly by an ``EasyList``. | ||
|
|
||
| Following the ``NewBase`` design, a ``unique_name`` that was | ||
| generated automatically is *not* written by ``to_dict``; a | ||
| deserialized descriptor is simply assigned a fresh one. Only a | ||
| ``unique_name`` passed explicitly to the constructor is serialized. | ||
|
|
||
| Descriptors no longer provide the ``SerializerComponent`` methods | ||
| ``encode``, ``decode`` and ``encode_data``. Use a serializer | ||
| directly instead, e.g. ``SerializerDict().encode(descriptor)`` and | ||
| ``SerializerDict.decode(data)``. |
There was a problem hiding this comment.
I wouldn't keep these comments. You can just look at the NewBase class to see its design, and I also wouldn't describe the difference from the old implementation.
|
|
||
| def __init__( | ||
| self, | ||
| name: str, |
There was a problem hiding this comment.
We want to get rid of the name attribute. This was part of the reason to move to NewBase.
| # Registers the descriptor with the global object map and takes | ||
| # care of `unique_name` and `display_name`. |
There was a problem hiding this comment.
Should we also get rid of parent? I think that is only used in the global_object map . . .
There was a problem hiding this comment.
a larger scope but fitting here so will add this
| @property | ||
| def display_name(self) -> str: |
There was a problem hiding this comment.
Can be removed when we remove name.
| # Only ModelBase members are accepted: EasyList harvests parameters | ||
| # from ModelBase items alone, so any other NewBase (a bare Parameter, | ||
| # say) would be accepted and then silently sit out the fit. | ||
| self._fit_objects = EasyList(*fit_objects, protected_types=ModelBase) |
There was a problem hiding this comment.
Hmm, I guess we would need to edit the EasyList to also return bare Parameters, otherwise we can't get rid of ObjBase to use the Fitter in standalone fitting.
There was a problem hiding this comment.
or maybe have Fitter.__init__ accept a plain list (or tuple) of parameters and wrap it in an EasyList itself? so users don't even need to know the container exists.
| `ModelBase`, `EasyList`; the legacy `ObjBase` and `CollectionBase` are | ||
| deprecated). |
There was a problem hiding this comment.
Remove line about legacy, they will be removed in 3.0, when this is released, so they won't even exist anymore.
| def test_is_a_new_base(self, descriptor: DescriptorBase): | ||
| # When Then Expect | ||
| assert isinstance(descriptor, NewBase) | ||
| assert not isinstance(descriptor, SerializerComponent) |
There was a problem hiding this comment.
legacy check, but can remove.
| def test_to_dict_drops_generated_unique_name(self, clear): | ||
| """Descriptors follow the ``NewBase`` design: an auto-generated | ||
| unique_name is not serialized, so a decoded descriptor is given | ||
| a fresh one instead of colliding with the original.""" | ||
| # When | ||
| descriptor = DescriptorNumber(name='name', value=1.0) | ||
|
|
||
| # Then Expect | ||
| assert descriptor._default_unique_name | ||
| assert 'unique_name' not in descriptor.to_dict() | ||
|
|
||
| def test_to_dict_keeps_explicit_unique_name(self, clear): | ||
| """An explicitly supplied unique_name is still serialized.""" | ||
| # When | ||
| descriptor = DescriptorNumber(name='name', value=1.0, unique_name='explicit_name') | ||
|
|
||
| # Then Expect | ||
| assert not descriptor._default_unique_name | ||
| assert descriptor.to_dict()['unique_name'] == 'explicit_name' |
There was a problem hiding this comment.
This is tested in NewBase, no need to test it again.
| def test_can_be_held_by_an_easy_list(self, clear): | ||
| """Descriptors are NewBase objects, so EasyList accepts them.""" | ||
| # When | ||
| descriptor = DescriptorNumber(name='name', value=1.0) | ||
| easy_list = EasyList(descriptor) | ||
|
|
||
| # Then Expect | ||
| assert list(easy_list) == [descriptor] | ||
| assert easy_list[descriptor.unique_name] is descriptor |
There was a problem hiding this comment.
Redundant test, I'd say. At least it should be covered by our integration tests, not a unit test.
I prefer to do it in a separate PR. The changes required are quite substantial. |
DescriptorBasenow inherits fromNewBaseinstead ofSerializerComponent, and usesSerializerBasefor serialization. Theto_dictmethod is introduced as the standard serialization method, withas_dictprovided as an alias for backward compatibility.The serialization interface for descriptor and parameter classes (
DescriptorNumber,DescriptorArray,DescriptorAnyType,Parameter) is unified to useto_dict, replacing previousas_dictimplementations.EasyListused inMultiFitternow only acceptsModelBasemembers, preventing bare parameters or foreign objects from being silently ignored during fitting.