add affine transform action mixin class

[roussel-ryan]

This pull request introduces a new mixin class to support affine transformations on variable actions in the lume/actions.py module. The main addition is the AffineTransformMixin, which allows actions to automatically apply scaling and offset transformations when getting or setting values, making it easier to work with variables that require such conversions.

Key changes:

Affine transformation support:

• Added AffineTransformActionMixin, a mixin for Action subclasses that applies an affine transformation (scaling and offset) to values when getting from or setting to a simulator. This includes:
  • Overriding the _get method to apply the inverse affine transformation when retrieving a value.
  • Overriding the _set method to apply the affine transformation before assigning a value, with error handling for unimplemented _set methods.

[tangkong]

These can't be optional in the current implementation (there's no None handling), and if we have default values that are functionally no-ops (scale=1, offset=0), there's no reason to allow these to be None

[electronsandstuff]

Retyping this as I looked again and got a better understanding of how the class works. The only thing that sticks out to me is:

• Confusion over method resolution order with the mixin. Could be resolved with lots of documentation.
• It requires users to create a new class type with the mixin every time they use it

One alternative is to make it a wrapper.

class AffineAction(WriteableAction, ScalarVariable):
    scale: float = 1.0
    offset: float = 0.0
    action: Action  # Needs work

  def _set(...)
    val = transform(val)
    action._set(val)

[tangkong]

If we're going to return to composition should we re-think the Actions Mixin approach as a whole? Switching back and forth between composition / inheritance is sure to be dizzying. I think the natural way one would extend this framework is by subclassing Action (following the pattern we lay out), so we're sure to run into this kind of issue again. (as we are in lume-impact currently)

The mro problems you're foreseeing are if you inherit from both this and ReadOnlyActionMixin or WritableActionMixin right? This action mixin lives at the same "generation" as those two (direct child of Action), so our recommendation could just be to not mix ActionMixins with each other.

[electronsandstuff]

Yeah, some of the problems are having confused users doing this:

class MyTransformedAction(AffineAction, ReadOnlyAction, ScalarVariable):
  # Will not call AffineAction._get / _set
  def _get(...):
    ...

or this:

# Correct
class MyTransformedAction(AffineAction, MyExistingAction): ...

# Won't call AffineAction._get / _set
class MyTransformedAction(MyExistingAction, AffineAction): ...

Specifically the last one is definitely something that would trip me up and unti the last few years I wouldn't even know to think of the ordering as an issue. I can see our users (mostly physicists who use software) finding this difficult to use and would prefer a solution that avoids it.

The suggestion isn't to return to composition. A wrapper action would fix some of these issues. Are there technical concerns here? We use mixins for adding in the ._get / ._set interface, but we don't need to tie ourselves to everything being a mixin from here unless there's a good reason it should look like one.

[tangkong]

I think we agree that the MRO specifics open the door to subtle and hard-to-diagnose errors. And I think we can brainstorm ways to make our lume subclasses more robust / clear. However we can't stop people from making these MRO mistakes in their isolated code bases if they're creating mixins similar to this.

My current best idea is to add __init_subclass__ hooks that disallow actions from being mixed in with each other at all. This makes the actions less "true" mixins, but could prevent this specific error mode in all descendants of Action

Details

class Action(ABC, Generic[SimulatorT]):
    """
    Parent class for testing if something is an Action.

    Do not subclass directly. Use ``ReadOnlyActionMixin`` or
    ``WritableActionMixin`` mixed into a ``Variable`` subclass.
    """

    def __init_subclass__(cls) -> None:
        action_found = False
        first_base = None
        for cls_base in cls.__bases__:
            if issubclass(cls_base, Action):
                if action_found:
                    raise TypeError(f"Cannot mixin unrelated Actions: ({first_base}, {cls_base})")
                action_found = True
                first_base = cls_base
        return super().__init_subclass__()
    

class ActionSub1(Action):  # OK
    pass

class ActionSub2(Action):  # OK
    pass

class ActionXOver(ActionSub1, ActionSub2):  # raises TypeError
    pass

[tangkong]

My comment from before was born from a desire to keep these classes simple and understandable. If we provide a mixin but break that pattern for a subset of the Actions, that would certainly confuse downstream users. ("use the mixins, unless you want that one mixin, then use a factory function that creates the class... etc")

Perhaps it's just my distaste for multiple inheritance and mixins leaking out again. We're already seeing a zoo of very specific subclasses that can't be inherited from in intuitive ways, and I was hoping we might be able to simplify while the idea is young.