[rozyczko]

Removed/replaced the legacy ObjBase and CollectionBase classes with the new EasyList and ModelBase classes.
Removed references to legacy APIs from documentation, configuration, and import statements.

Removed Hugger and Property from global_object\hugger

Removed Models/polynomial.py and associated tests. This required adding a stub model in tests/helpers/easy_test_models.py

Changed EasyList.to_dict to EasyList._convert_to_dict, because an override of to_dict only runs when someone calls easy_list.to_dict() directly. It never runs when the list is nested inside another object.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.11%. Comparing base (c38c0ea) to head (970f169).

Additional details and impacted files

@@                 Coverage Diff                  @@
##           308-remove-name     #314       +/-   ##
====================================================
+ Coverage            83.44%   94.11%   +10.66%     
====================================================
  Files                   69       52       -17     
  Lines                 5177     4128     -1049     
====================================================
- Hits                  4320     3885      -435     
+ Misses                 857      243      -614     

Flag	Coverage Δ
unittests	94.11% <100.00%> (+10.66%)	⬆️

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

Files with missing lines	Coverage Δ
src/easyscience/__init__.py	100.00% <100.00%> (ø)
src/easyscience/base_classes/__init__.py	100.00% <100.00%> (ø)
src/easyscience/base_classes/easy_list.py	96.89% <100.00%> (+0.62%)	⬆️
src/easyscience/base_classes/model_base.py	100.00% <ø> (ø)
src/easyscience/base_classes/new_base.py	100.00% <100.00%> (ø)
...syscience/fitting/calculators/interface_factory.py	97.32% <100.00%> (+0.04%)	⬆️
...c/easyscience/fitting/minimizers/minimizer_base.py	97.95% <ø> (ø)
.../easyscience/fitting/minimizers/minimizer_bumps.py	99.35% <ø> (ø)
...rc/easyscience/fitting/minimizers/minimizer_dfo.py	97.74% <ø> (ø)
.../easyscience/fitting/minimizers/minimizer_lmfit.py	98.43% <ø> (ø)
... and 12 more

... and 2 files with indirect coverage changes

[rozyczko]

Replacement of EasyList.to_dict to EasyList._convert_to_dict:

An override of to_dict only runs with direct calls to easy_list.to_dict(). It never runs when the list is nested inside another object.

When a parent object is serialized, SerializerBase._convert_to_dict parses the parent's constructor args. Each child value goes through _recursive_encoder. For an easy* object, that calls encoder._convert_to_dict(). A nested EasyList (i.e. the old Polynomial.coefficients) was encoded, but its data and protected_types were silently dropped. It could not be decoded back.
That hook runs both:

• a direct EasyList.to_dict(), which goes through the inherited NewBase.to_dict(), then SerializerBase._convert_to_dict, then the hook;
• a nested list, which goes through _recursive_encoder, then SerializerBase._convert_to_dict, then the hook.

So moving the list-specific logic means the items are written in both cases. It also helps EasyList when duplicating what NewBase.to_dict() already does.

Also: skip now reaches the list

The old override was to_dict(self) with no skip parameter. NewBase.copy calls self.to_dict(skip=['unique_name']), which would have raised a TypeError on an EasyList.

The original ignored skip and still called item.to_dict() for each item. The current version passes skip on to each item and gives each item its own copy (list(skip)), because NewBase.to_dict() appends to the list it is given.

[damskii9992]

Replacement of EasyList.to_dict to EasyList._convert_to_dict:

An override of to_dict only runs with direct calls to easy_list.to_dict(). It never runs when the list is nested inside another object.

When a parent object is serialized, SerializerBase._convert_to_dict parses the parent's constructor args. Each child value goes through _recursive_encoder. For an easy* object, that calls encoder._convert_to_dict(). A nested EasyList (i.e. the old Polynomial.coefficients) was encoded, but its data and protected_types were silently dropped. It could not be decoded back. That hook runs both:

• a direct EasyList.to_dict(), which goes through the inherited NewBase.to_dict(), then SerializerBase._convert_to_dict, then the hook;
• a nested list, which goes through _recursive_encoder, then SerializerBase._convert_to_dict, then the hook.

So moving the list-specific logic means the items are written in both cases. It also helps EasyList when duplicating what NewBase.to_dict() already does.

Also: skip now reaches the list

The old override was to_dict(self) with no skip parameter. NewBase.copy calls self.to_dict(skip=['unique_name']), which would have raised a TypeError on an EasyList.

The original ignored skip and still called item.to_dict() for each item. The current version passes skip on to each item and gives each item its own copy (list(skip)), because NewBase.to_dict() appends to the list it is given.

Our serialization contract is to_dict and from_dict, not convert_to_dict. We need to change the line in SerializerBase which checks if the object has a _convert_to_dict method and replace that with a to_dict method.

This is the correct approach, not removing to_dict. The fix should be simple:
Simply replace "_convert_to_dict" with "to_dict" in lines 254 and 255 in serializer_base.py.

[damskii9992]

I already wrote my main comment about the to_dict method, but that needs changing. Otherwise only some minor stuff.

[damskii9992]

I'm totally for removing this (and the above) branch of code, which tries to get the attribute from a public and internal "kwargs" method, but why remove it here? What is the reason you're removing it and why only the internal one?

[rozyczko]

Our serialization contract is to_dict and from_dict, not convert_to_dict. We need to change the line in SerializerBase which checks if the object has a _convert_to_dict method and replace that with a to_dict method.

This is the correct approach, not removing to_dict. The fix should be simple: Simply replace "_convert_to_dict" with "to_dict" in lines 254 and 255 in serializer_base.py.

Nope.

This hook is a post-processing callback, not an entry point, so swapping the names fails in three ways:

• Wrong signature. The hook is called as (dict_repr, encoder, skip=…), but to_dict only takes (skip=None). If you try it you get: TypeError: Parameter.to_dict() got multiple values for argument 'skip'.
• Infinite recursion. Every NewBase has to_dict, so hasattr is always true. The call chain would be NewBase.to_dict -> SerializerBase._convert_to_dict -> obj.to_dict -> and round again.
• Different meaning. The hook adds to a dict that is already half built. to_dict builds a whole new dict from scratch.

On the other hand... "Our contract is to_dict/from_dict" is a good design idea.
Right now _recursive_encoder goes to encoder._convert_to_dict for nested Easy objects. That skips every to_dict override, not just the one on EasyList.
So my _convert_to_dict hook fixes EasyList only. Nested Parameter and DescriptorNumber objects will still lose what their to_dict overrides add (dependency info, __serializer_id etc).
This is nor a newly introduced bug but needs addressing (separate issue)

Maybe we should make the to_dict branch in _recursive_encoder call the actual object method?

elif hasattr(obj, 'to_dict') and obj.__class__.__module__.startswith('easy'):
    return obj.to_dict(skip=list(skip))

this way nested Parameter will keep its dependency fields

[rozyczko]

Meh, there's still an issue with display_name serialization

from easyscience import Parameter
from easyscience.base_classes import EasyList

# A list with no display_name, with an item that has display_name.
el = EasyList(Parameter(1.0, display_name='c0'))

d = el.to_dict()
print('display_name' in d['data'][0])        # False  (expected True)

# now go to_dict->from_dict and the name is gone, it gets the default
el2 = EasyList.from_dict(d)
print(el2[0].display_name)                   # Parameter_1  (expected 'c0')

# Same list, but give the list itself a display_name: the item now keeps its name.
el3 = EasyList(Parameter(1.0, display_name='c0'), display_name='coeffs')
print('display_name' in el3.to_dict()['data'][0])   # True

[damskii9992]

Meh, there's still an issue with display_name serialization

Well, you introduced this one. The _convert_to_dict gets called with the built skip list, and display_name gets appended to the skip list when there is no display_name set on the parent class. The previous to_dict implementation did not suffer from this issue, as each parameter evaluated themselves whether display_name needed to be added to the skip list.

[damskii9992]

Maybe we should make the to_dict branch in _recursive_encoder call the actual object method?

elif hasattr(obj, 'to_dict') and obj.__class__.__module__.startswith('easy'):
    return obj.to_dict(skip=list(skip))

this way nested Parameter will keep its dependency fields

Yes, I think we should think of serialization this way:

If an object is our own object and it has a to_dict method, call that method to serialize it.
We then only use the generic serializer otherwise.

This way we can control how our own objects gets serialized by overwriting to_dict wherever necessary, like for EasyList.

[damskii9992]

You're removing crucial serialization functionality here, check out my pr #325 which should be doing what we want the NewBase to_dict method to do.

[damskii9992]

Should we move SerializerBase to our base_classes module now that it is the only class in this module?

[rozyczko]

will do that on #313 after merge, so it's easier