Skip to content

WIP: allow model parameters to be passed into model element constructors

finesse importer requested to merge feature/model-parameter-separate-init into master

This changes the way model parameters are created and added to model elements to allow !39 (closed) to work.

Model parameter objects for an element are currently created in __new__ and assigned default values before __init__ gets called. Then any values passed into __init__ for these parameters (R, T, etc.) are set in the associated model parameter object. There is no way to set an element's model parameter object directly, but this is something that will be required by the parser in order to support self-referencing parameters like m m1 R=1-&m1.T T=0.01. The changes required to support this (see !39 (closed)) involve building Parameter objects before their associated model elements, and this is what this MR changes.

Now model parameters are created during ModelElement.__init__, not __new__. This has wide-ranging effects on code that previously relied on the old behaviour, noted below.

Noteworthy changes

Where model parameters now get created

Model parameters used to be created in __new__, but are now created in ModelElement.__init__. This gives child classes the opportunity to set a model parameter object directly, and thus allows the parser to pass such pre-built parameters as arguments. If a built Parameter is not passed, the same behaviour as before is used: a new Parameter is built and assigned either the passed value (if it's a float or whatever) or the default.

Owning elements are no longer set in the Parameter constructor

Parameter currently needs its owning component passed into its constructor. This MR changes it to not take the owner during its own __init__, but rather to have it be assignable as a property. The owning component is not currently required in __init__ as far as I can tell; it's only needed later, e.g. to get the parameter's name (for repr etc.) and its associated model.

Setting model parameter values in __init__

Element constructors now need changed to pass their model parameters to ModelElement.__init__. Currently they usually just set their own fields directly, like self.phi = phi, but this bypasses the checks in ModelElement.__init__ to determine if the passed value is actually an already-built Parameter as would be the case with the parser. The constructors for all public elements therefore need changed to forward the parameter values passed into the constructor up to ModelElement.__init__.

Default values for parameters

Currently the model parameters had two defaults: the default specified in the @model_parameter decorator, and the default in the __init__ signature. The first was being overridden by the second in most cases, making the default defined in the model parameter mostly useless. Now model parameters don't have defaults defined any more, and are always passed their value one way or the other by ModelElement.__init__, either using the default specified in the concrete class's constructor signature or using the value specified by the user.

Surface class

The Surface class, which implements common functionality between Mirror and Beamsplitter, no longer needs @model_parameter decorators of its own - these were never used, nor useful, since they had to be defined on Mirror and Beamsplitter anyway.

Things that will break and need fixed

Mirror mass and signal gain

Mirror previously didn't have mass and signal_gain @model_parameter decorators, but did support these arguments in the signature. When I added support for these arguments as decorators, I got an error with the modal simulation code - I think it doesn't handle these parameters properly, even though it accepted them. I therefore removed them for now but we should probably add them back.

Model elements can't inherit from ABC

For some reason, if a model element child class (e.g. Detector) has an ABCMeta metaclass, or is a direct subclass of ABC, it breaks some Cython type definition:

This has been fixed.

________________________________________________ ERROR at setup of test_propagation__frequency_offset[3141] ________________________________________________

model = <finesse.model.Model object at 0x7fa64aa29670>

    @pytest.fixture
    def laser_and_mirror_with_ad(model):
        """Model with a laser and mirror separated by a space, with amplitude detector."""
        model.chain(Laser("L0"), Space("s0"), Mirror("M", R=1, T=0))
>       model.add(AmplitudeDetector("ad_lsr", model.L0.p1.o, model.L0.f.ref))

tests/functional/test_space_phase_propagation.py:16: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
src/finesse/detectors/amplitude_detector.py:43: in __init__
    Detector.__init__(self, name, node, dtype=np.complex128, label="Amplitude", f=f)
src/finesse/detectors/general.py:45: in __init__
    ModelElement.__init__(self, name, **kwargs)
src/finesse/element.pyx:103: in finesse.element.ModelElement.__init__
    parameter = create_model_parameter(self.__class__, name, value)
src/finesse/element.pyx:50: in finesse.element.create_model_parameter
    return type_(name, description, value, unit, flag, cls)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   self.__component_type = component_type
E   TypeError: Expected type, got ABCMeta

This was patched by removing all inheritance from ABC, but this is obviously desirable to enforce the use of only public APIs.

Cross-references with the Python API

The user can in the current Python API in master not do this:

>>> from finesse.components import Laser
>>> x = Laser("l1")
>>> y = Laser("l2", P=x.P)
Exception: Unexpected input '1 W'

If they want to make a reference to x.P in y they are supposed to append .ref to the parameter, like this:

>>> from finesse.components import Laser
>>> x = Laser("l1")
>>> y = Laser("l2", P=x.P.ref)
>>> y.P
<l2.P=l1.P @ 0x7fef76f82a60>

With the changes in this MR, the first example above will currently be allowed, and will set l2's P parameter to point to l1's P directly rather than via a reference. This is probably not what the user intends, and probably breaks the simulation code somehow since parameters are supposed to be component-specific instances. We should think of some way around this. One idea would be to have a special flag in ModelElement.__init__ which switches off the checks that the passed object is not already a Parameter, thus allowing the parser to still work:

>>> from finesse.components import Laser
>>> x = Laser("l1")
>>> y = Laser("l2", P=x.P)
Exception: Unexpected input '1 W'
>>> y = Laser("l2", P=x.P, _allow_unref_params=True)  # In reality, the flag would only be set by the parser, never elsewhere.
>>> y.P
<l1.P=1 @ 0x7fef76f82550>

It would obviously be silly for the user to use this flag directly, but the parser would be able to use it to get the correct behaviour.

Alternatively we could define a separate init method altogether, which only the parser would use, and the public API would be kept the same (not allowing existing model parameters as arguments).

Or, the parser could set some special field on whatever the element is, e.g. Beamsplitter._dont_check_for_references = True before calling Beamsplitter.__init__ then resetting it afterwards (context mananger?).

Update from the 2020-11-05 telecon

We decided to have two behaviours depending on the interface. In kat script, specifying R=x.T would copy x.T's value to R and pass the value (float or whatever) to the Python constructor. Using the Python API directly, if the user specified R=x.T it would throw an error saying the user should specify either .value or .ref.

How could this alternatively be achieved?

I suppose another way would be to keep everything as it current is, except allow Parameter values to be set to other parameters. Then when the Parameter value setter would see a Parameter passed, it would overwrite itself with the passed parameter. This seems clunky to me though - creating a Parameter in __new__ that gets immediately overwritten with whatever the user passes into __init__, and it would cause derived things like ABCD matrices to get recomputed too. It might also break some weakrefs.

Another way could be to have the parser set some field of the element in question, _use_these_model_parameters, and __new__ will then use them instead of creating defaults. Again, seems clunky.

Todo

  • Move parameter creation from __new__ to __init__, and use the passed values instead of defaults if given.
  • Change all element constructors to pass arguments to ModelElement.__init__.
  • Get this working with setRTL in Mirror, Beamsplitter, etc. and in any other places where model elements are validated before being set in __init__.
  • Add mass and signal_gain model parameters to Mirror?
  • Fix ABC issue.
  • Allow strings to be set for parameters expecting ints/floats (see #194 (closed))
  • Document the behaviour when calling the constructor in Python with various parameter syntaxes: R=x.T, R=x.T.value, R=x.T.ref, R=float(x.T), R=1*x.T, etc.
  • Document the copy by value/reference behaviour when using kat script
  • Emit log messages in above cases when parameters get converted to values
  • (From telecon) Change component setter in Parameter to something like _unsafe_set_owner, since we don't want users calling this setter
Edited by finesse importer

Merge request reports