OrderedDescriptor and OrderedDescriptorContainer - #3679

Any thoughts about this?

Sorry -- I had meant to have a look at this and haven't had a chance (and this week is awful for me). Don't want to hold up the works, though.

Sorry also been busy but I like the idea and simplification in coordinates. Will try for a more detailed look this week.

Ack, it's okay. Now that I'm in it, this week is awful for me too; I don't expect I'll need this 'til after Leiden.

I haven't attempted to understand all the details of the new code, but I note that it is extremely well documented and so the subtleties should be transparent to future maintainers!

From the coordinates perspective I've looked through the changes and confirmed that things look reasonable. Given that tests are passing I'm 👍 on this. @eteq - do you want to weigh in on this?

I guess to help guide any further review, there are two main things about this I'm not 100% certain about and would like feedback on (and these can always be changed later if need be). Both have to do with how the Descriptor class hooks up with classes it's used on.

The first is the _class_attribute_ option. This attribute is defined on the Descriptors, but it requires the class using those descriptors to have an attribute as defined by _class_attribute_ which may be seen as a bit incongruous. An alternative might be for the class using the descriptors to specify the name of an attribute that each descriptor class is tied to. To give an example from coordinates, currently we write:

class FrameAttribute(OrderedDescriptor):
    _class_attribute_ = 'frame_attributes'
    # ...

class FrameMeta(OrderedDescriptorContainer):
    #...

class BaseFrame(metaclass=FrameMeta):
    # Default empty frame_attributes; in principle it's not necessary to define
    # this default, but in practice it's pretty convenient to, and makes more clear
    # that this attribute exists (otherwise it will come in 'magically' via the metaclass
    frame_attributes = OrderedDescriptor()

This of course means that if I wanted to use FrameAttribute in another class, but I wanted it use a different class attribute than frame_attributes, I'd have to subclass FrameAttribute and override the _class_attribute_. A don't think that's a huge problem myself, but I can see how one might find it confusing. An alternative would be to specify this mapping in the class that uses the descriptors. For example it could specify up front which OrderedDescriptors it wants to collect. There would be no FrameAttribute._class_attribute_, and instead:

class BaseFrame(metaclass=FrameMeta):
    _ordered_descriptors_ = {FrameAttribute: 'frame_attributes'}
    frame_attributes = OrderedDict()

etc. This would map ordered descriptor classes to the names of attributes on the class. It might also be able to automatically set up the default, so that the last line in the above example is unnecessary.

The other question is what the default behavior should be for inheritance. I chose, as the default, for subclasses to not inherit OrderedDescriptor collections from their parent classes, since this is the (current) behavior of Model. But I had to add an option for a different inheritance mechanism to support the existing behavior of BaseFrame. I don't really care which is the default--I think whichever makes more sense.

@embray

On the first question, I prefer the current implementation over the option of something like _ordered_descriptors_ = {FrameAttribute: 'frame_attributes'}. The current version seems more obvious to me.

On the second question, my natural inclination would be for the ordered descriptor collections to inherit from their parent classes, but it's not a huge deal either way.

@eteq As a stakeholder in the coordinates package do you have any opinion on this (or other stakeholders in coordinates?)

I think overall the changes to coordinates in this are very minimal, and mostly result in code simplification. As far as I can tell the behavior of frame attributes is still correct.

Rebased on master. Going to go ahead and merge this soon, after I add a little more documentation.

👍

Rebased this again--going to go ahead and merge soon as I get the green light from CI.

I hoped that this would improve some of the problems with subclassing models, but it's still tricky due to deeper reasons (though it will help). I'll go ahead with this as is and address the model subclassing problems separately (#4035).