ADR suggestion: name, EasyDynamicsList and ComponentCollection

[henrikjacobsenfys]

General

I want to be able to select components (and other things, but this is about components) by their name instead of their index. For example

import easydynamics.sample_model as sm
from copy import copy
gaussian1 = sm.Gaussian(name='gaussian1')
gaussian2 = sm.Gaussian(name='gaussian2')
collection1 = sm.ComponentCollection(components = [gaussian1, gaussian2])

collection2 = copy(collection1)
collection2_gaussian1 = collection2['gaussian1']
print(gaussian1)
print(gaussian1.unique_name)
print(collection2_gaussian1)
print(collection2_gaussian1.unique_name) 

Gaussian(name = gaussian1, display_name = gaussian1, unit = meV,
    area = <Parameter 'gaussian1 area': 1.0000 meV, bounds=[0.0:inf]>,
    center = <Parameter 'gaussian1 center': 0.0000 meV (fixed), bounds=[-inf:inf]>,
    width = <Parameter 'gaussian1 width': 1.0000 meV, bounds=[1e-10:inf]>)
Gaussian_0
Gaussian(name = gaussian1, display_name = gaussian1, unit = meV,
    area = <Parameter 'gaussian1 area': 1.0000 meV, bounds=[0.0:inf]>,
    center = <Parameter 'gaussian1 center': 0.0000 meV (fixed), bounds=[-inf:inf]>,
    width = <Parameter 'gaussian1 width': 1.0000 meV, bounds=[1e-10:inf]>)
Gaussian_2

(Obviously this particular example isn't very useful, but it's just to show the idea).

unique_name does not work for this, since it is not copied. So I need a name that is unique only to the list, also as discussed in easyscience/core#194.

To do this, I need to create EasyDynamicsList (which inherits from EasyList) using name instead of unique_name for the lookup, and let ComponentCollection inherit from it.

A draft is found in #185

NameMixin

I now have three classes that need name: EasyDynamicsBase (inheriting from NewBase), EasyDynamicsModelBase (inheriting from ModelBase) and EasyDynamicsList (inheriting from EasyList). They all need name, so I make a NameMixin.

People should in general be allowed to assign their components a new name. However, since name has to be unique within a list, they should not be able to assign it to a name that is already in the list. The difficulty here is that a component can in principle be a member of an arbitrary number of lists.

Checking that a new name doesn't clash with all lists is a pain to implement and not worth it. Instead, I will lock the name if it's in a list, and unlock it if it's removed from the list.

The lock status will be an integer and not a boolean. If an object is added to two lists, the lock value will therefore be 2. If the object is removed from one list the lock value is reduced by 1. The name can only be changed if the lock value is 0.

EasyDynamicsList

Inherits from EasyList.
Needs to overwrite the insert, append, extend, __setitem__ with validation that name is unique to the list and locking the name, and pop, __delitem__ with unlocking the name. _get_key will also be overwritten to use name.

ComponentCollection

Inherits from EasyDynamicsList and EasyDynamicsModelBase (for unit and name). protected_types will be ModelComponent. Some methods need to be updated, and a lot of code needs to be updated to handle the new API.

[seventil]

Following a lengthy discussion we had IRL about the root of these requirements, from my understanding:

The user needs to have unique names to correctly refer components in components collection.

I think that the user would make uniquely looking names anyway, unless they really really want to complicate their life. I suggest that you just let those who want to make things difficult for themselves to do so. There will be immediate feedback that they can fix with another rename. IMHO It's not worth to introduce complicated logic to guard weird user agency if it does not lead to erroneous state.

It is similar to the following standard lib code:

a = list["one", "two", "three"]
a[2] = "one"
a.index("one") # == 0

If i see someone doing that and then complaining that print(a) output ["one", "two", "one"] is unreadable, i would do nothing.

Stuff below is just me trying to rationalize the argument above, feel free to ignore it. I include it.

We essentially have few cases, when that it important:
First case is pretty simple.

g1 = Gaussian(name="gaussian")
g2 = Gaussian(name="gaussian")
col1 = ComponentCollection(components = [g1, g2])

When user creates the collection and tries to put non-uniquely named components into it - the visual indication on, say, legend of a plot would give the user feedback that they can't actualy refer to them and the user can now just rename one component to make it usable. It was their own decision to make that weird and they can easily fix that.

The second one resembles the first one

g1 = Gaussian(name="g1")
g2 = Gaussian(name="g2")
col1 = ComponentCollection(components = [g1, g2])

col2["g2"].name = "g1"

This is again weird agency, but easy to fix by the user anway based on visual indication of, what essentially is col1.names

The last one is an edge case.
When the user creates a list with uniquely named components, places a reference of the same component in another list with differently named component so that this component will now clash with the first. Essentially

g1 = Gaussian(name="g1")
g2 = Gaussian(name="g2")
col1 = ComponentCollection(components = [g1, g2])
col2 = ComponentCollection(components = [g2])

col2["g2"].rename("g1")

Raising exception on the rename requires complicated logic.

Essentialy, for the sake of ensuring something that the user whould naturally want and make themselves anyway. Without the guard, the rename might make a hard-to-read state somewhere else but only if the user initially makes a complex composition of components in the first place.

My expectation would be, that if I'd create a complex composition of components, I would want to name the components such that I could easily categorically differentiate between them, meaning that even if I use g2 in different places, I wouldn't want to rename it to g1, where g1 is not present. It would be weird user agency, but with clear feedback when they need it and easily fixable.

[rozyczko]

So the main problem is really:
We need a convenient way to retrieve a component from a ComponentCollection by its user-modifiable name.
Names should not be globally unique. Ambiguity should be handled locally by the collection at lookup time.

I would object to Python list .index() behavior as the default for name lookup.
Python’s list.index() returning the first match is fine because lists do not claim semantic uniqueness.
But collection["name"] visually suggests dictionary-like lookup by a key, and dictionary-like lookup strongly implies uniqueness.

let users make duplicate names; visual feedback will tell them; they can fix it. - I do not agree with this sentiment. This is acceptable for things like plot legends but not for programmatic mutation.
E.g.

collection['gaussian'].width = 3.0

will silently return the first component only, if there is more than one 'gaussian'

What I would suggest is to not implement name-locking or global tracking.
But also would not silently allow ambiguous name lookup to return the “first” match unless that behavior is very explicitly documented.

No component should need to know which collections contain it.

Instead, why not just be permissive with names but balk/raise on the actual query?

g1 = Gaussian(name="g")
g2 = Gaussian(name="g")
col = ComponentCollection([g1, g2])

col["g"]   # raises AmbiguousNameError
col[0]     # works
col[1]     # works

We could access the components with additional helper methods like

collection[0]              # index lookup
collection["gaussian1"]    # name lookup if unambiguous
collection.names           # diagnostic
collection.duplicate_names()  # diagnostics for dupes

This makes the implementation MUCH less complex/error prone with no locking or having the components know their parents/containers.

[seventil]

I agree that square brackets suggest uniqueness and I wanted to add that it might be a good idea to have collection.has_duplicate_names property or something like that but it didn't tie in with my suggestion. AmbiguousNameError/DuplicateNamesError/KeyError is the way to go

[henrikjacobsenfys]

why not just be permissive with names but balk/raise on the actual query?

This seems like a good idea that I just hadn't thought of.