Transitive rebinding

[charlesbaynham]

Adds support for transitive rebinding of parameters.

If fragment A has two children, B and C, support already existed for overriding C.my_param to depend on B.my_param then B.my_param to depend on A.my_param.

This PR lets you do it the other way around, giving the same final result but reversing the call order so you can first bind B.my_param to A.my_param, then C.my_param to B.my_param.

We found this need pop up when wanting to bind parameters between subfragments at the same hierarchical level.

[dnadlinger]

@charlesbaynham Thank you very much for the PR, and apologies for letting it languish here for so long. I completely missed that this was still unanswered all this time.

I'm not quite convinced I understand why this should be necessary. In the example of rebinding between multiple sibling fragments, doesn't the parent know which is supposed to go where?

Regardless, I'm not in principle opposed to relaxing the restrictions here. In that context, though, isn't it a bit unnatural to keep distinguishing between rebound parameters and those that are just overridden? Surely, in so far as anything matters, it's that it is no longer a free parameter of the source allow rebinding to other fragments? In other words, shouldn't this just handle already rebound parameters too?

(Also merging 10fa285 as #417; could you just rebase this PR to drop it? (happy to fix that, but I'm assuming you are very familiar with Git))

[charlesbaynham]

Hey @dnadlinger , no problem in the slightest; releasing helpful libraries publically is a good deed that is always punished by more work.

(Also merging 10fa285 as #417; could you just rebase this PR to drop it? (happy to fix that, but I'm assuming you are very familiar with Git))

Done. I've just rebased to master, I assume you'll squash all these commits when merged.

I'm not quite convinced I understand why this should be necessary. In the example of rebinding between multiple sibling fragments, doesn't the parent know which is supposed to go where?

The context for this request is a pattern that has developed in our code. We've found ourselves using a structure where a base class implements a generic experiment and then specialises it by inheriting from mixin classes. So for example, we might have a "load a red MOT and then do something to it" experiment which has "hooks" like do_something_to_atoms_hook(), imaging_hook(), post_red_narrowband_hook()` etc, all of which are defined to be no-ops. Our mixin classes then implement these, resulting in most of our experiments looking like e.g.:

class ClockSpecFromXXODTFrag(
    ClockRabiSpectroscopyDipoleTrapMixin,
    DoubleTrapImagingNormalised,
    EMGain,
    FLIRBlueMOTMeasurementMixin,
    XODTSingleMolassesPlusFieldRampMixin,
    OpticalPumpingWithFieldSettingDipoleTrapMixin,
    DipoleTrapWithExperiment,
):
    """
    Clock spectroscopy from dropped XXODT

    Load into an XXODT, then use the up clock beam for spectroscopy, altering the
    (single-pass) AOM.

    Image the ground state atoms, repump and image the excited state, then image
    once more for background.
    """

    @kernel
    def before_start_hook(self):
        self.before_start_hook_clockspec()
        self.before_start_hook_xodt_molasses()

...with the actual behaviour implemented in the mixins. That leads us to wanting to extend existing sequences sometime but not other times, so I might have a Fragment (call it RampFrag) which implements a ramp in one part of the experiment and I'd like to bind some parameters of this ramp to parameters of the previous ramp. The easiest way is to have RampFrags daisy-chain themselves from one to the next, but this requires me to bind parameters to a previous RampFrag whose params are already bound. I could instead do all the binding at once, but then it would be messier to support sequences with differing numbers of RampFrags.

That's probably an unnecessairy level of detail to motivate this PR. In general, I think it's just nice to support binding in whatever direction the user fancies without restrictions on the order in which binding methods are called.

isn't it a bit unnatural to keep distinguishing between rebound parameters and those that are just overridden? Surely, in so far as anything matters, it's that it is no longer a free parameter of the source allow rebinding to other fragments? In other words, shouldn't this just handle already rebound parameters too?

Hmm, yes that does seem like the most general behaviour. I made this PR as a minimal extension, but yes that probably is better. If I'm understanding the code correctly then there's a slight difference between binding to an overridden / rebound param. The behaviour in both cases should be:

Binding to an already-bound param

• Follow the chain to locate the free param which the target param is rebound to
• Record the new target in _rebound_own_params
• Remove this ParamHandle from _free_params

vs Binding to an overridden-param

• Follow the chain to locate the top-level param which the target param is rebound to. This is not a free param, because it has been overridden
• Record the new target in _rebound_own_params
• Set the store for this ParamHandle to match the store for the target, top-level ParamHandle
• Remove this ParamHandle from _free_params

Do you agree? If so, I can implement this.

PS I think we should rename _rebound_own_params and _rebound_subfragment_params to _own_params_rebound_to_others and _own_params_targetted_by_rebindings or something like that. _rebound_subfragment_params is out of date since it doesn't necessarily point at subfragments.

[dnadlinger]

Thanks for the explanation of your use case. While I think about that some more: Yes, the idea is just always to bind to the ultimate parameter. If the target already has a store (overridden), then just set the store of the rebound handle to the target's. If it doesn't yet have a store (not overridden), we can only do this later, so we need to keep track of the fact that the rebind happened to enact it down the line.

Maybe instead of storing this in Fragment dicts, just inlining this tree in ParamHandle is cleaner? Each instance would have a ParamHandle | None link to the parent in the tree (if any), or a set/list/…[ParamHandle] list of children. In fact, it seems like as soon as the parent ("self is rebound to …") link is set, the list of children ("these are rebound to self") is no longer relevant (and can be cleared), as all the children would have been (transitively) forwarded to the ultimate parent already.

This would help with the naming problem too.

[dnadlinger]

(If you think it will be a while until you get a chance to implement this, I can also give it a shot.)

[charlesbaynham]

Maybe instead of storing this in Fragment dicts, just inlining this tree in ParamHandle is cleaner? Each instance would have a ParamHandle | None link to the parent in the tree (if any), or a set/list/…[ParamHandle] list of children. In fact, it seems like as soon as the parent ("self is rebound to …") link is set, the list of children ("these are rebound to self") is no longer relevant (and can be cleared), as all the children would have been (transitively) forwarded to the ultimate parent already.

That seems very elegant, sounds great to me. Don't we need to keep the list of children around so that overrides can set the store of all the children that point at them?

I'll have a go at this at some point but yes, it might be a while: do feel entirely free to take over, it's your library!

[dnadlinger]

Yes, one would need ot keep a list of children around.

This is strictly a refactoring opportunity, though (to avoid the naming difficulties), and can be done later down the line. For now, I'd be happy to just merge your implementation after removing the restriction to non-overridden parameters.

[charlesbaynham]

Hey @dnadlinger , I was stuck at home with a fever so I found some time to do this. I've implemented the moved logic you mentioned, that resulted in much cleaner code in Fragment.

Rather than attempting to point the children at the ultimate target, I've always kept a record of which ParamHandle was rebound to which, even if the direct parent ultimately gets rebound itself. That way one can reconstruct the bindings which might be needed for some reason in the future, and it's easier to write too.