Skip to content

Collection base easylist rebased#228

Open
rozyczko wants to merge 8 commits intodevelopfrom
collection_base_easylist_rebased
Open

Collection base easylist rebased#228
rozyczko wants to merge 8 commits intodevelopfrom
collection_base_easylist_rebased

Conversation

@rozyczko
Copy link
Copy Markdown
Member

This is the collection list as described in #226
Rebased against the new develop branch and due to the number of files affected, made a separate PR.

For discussion look in: #226

@rozyczko rozyczko added [scope] enhancement Adds/improves features (major.MINOR.patch) [priority] medium Normal/default priority [area] base classes Changes to or creation of new base classes labels Mar 18, 2026
@rozyczko
Copy link
Copy Markdown
Member Author

rozyczko commented Mar 18, 2026

Another question:
Currently, the code allows backward compatibility, so old Parameter and Descriptor objects can be used alongside the new ones based on NewBase.
Does it make sense to keep this compatibility layer for the interim process or should we completely forget about ObjBase derived classes?

@rozyczko
Copy link
Copy Markdown
Member Author

Another question: Currently, the code allows backward compatibility, so old Parameter and Descriptor objects can be used alongside the new ones based on NewBase. Does it make sense to keep this compatibility layer for the interim process or should we completely forget about ObjBase derived classes?

I don't think it makes sense to carry over functionality which will be removed in the next release.
Removed all vestiges of BasedBase dependencies, inclusing methods for backward compatibility with the old ModelCollection.

Copy link
Copy Markdown
Member

@henrikjacobsenfys henrikjacobsenfys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a stupid question, but shouldn't this inherit from or use EasyList? There is at least some duplication of logic.

Otherwise a handful of nitpicks and questions to consider.

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

self._protected_types = self._normalize_protected_types(protected_types)
self._name = name if name is not None else self.display_name
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If display_name and name are both None, then self._name is also None. Is that desired, or should there be a fallback?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't thing this is an issue. When name is None, self._name falls back to self.display_name, which in turn falls back to self.unique_name (auto-generated) in new_base.py: 83-86. So _name is never None.

if not isinstance(new_name, str):
raise TypeError('Name must be a string')
self._name = new_name
self.display_name = new_name
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that actually desired behavior? I would think that people want to keep name and display_name separate?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. The constructor already syncs them (if display_name is None and name is not None: display_name = name )
The setter keeps them in sync for consistency. If a user explicitly sets a different [display_name] via the constructor, the two diverge until name is set again.

This could be revisited if use cases emerge where they need to stay independent after initial construction.

@rozyczko
Copy link
Copy Markdown
Member Author

This may be a stupid question, but shouldn't this inherit from or use EasyList? There is at least some duplication of logic.

CollectionBase inherits from ModelBase (which provides parameter aggregation, get_all_parameters(), get_free_parameters(), etc.), while EasyList inherits from NewBase.

EasyList is a pure typed container. It holds a flat list of NewBase items in _data, enforces protected_types, and provides list operations + unique-name lookup. It has no awareness of models, parameters, or fitting — it's essentially a list[T]with type safety and serialization.

The new CollectionBase is a model-aware collection. Inherits from ModelBase and gets the full parameter aggregation API (get_*_parameters), running recursively. It also adds name, display-name lookups, and can have its own deserialization. This means a CollectionBase can be directly used in fitting.

@henrikjacobsenfys
Copy link
Copy Markdown
Member

Looks good to me now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[area] base classes Changes to or creation of new base classes [priority] medium Normal/default priority [scope] enhancement Adds/improves features (major.MINOR.patch)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants