BUG: additional pseudo-positioner (not in _pseudo) feature not working

[prjemian]

Edited to change title and emphasize the actual problem to solve.

Looks like a problem in ophyd.pseudopos, lines 417-8, the _idx is clearly set to None with statement that it will be set later:

        # The index of this PseudoSingle in the parent PseudoPositioner tuple
        # will be set post-instantiation:
        self._idx = None
        self._parent.subscribe(self._sub_proxy_start, event_type=self.SUB_START)
        self._parent.subscribe(self._sub_proxy_done, event_type=self.SUB_DONE)
        self._parent.subscribe(self._sub_proxy_readback,
                               event_type=self.SUB_READBACK)

Later, it would be set by this code:

        for idx, pseudo in enumerate(self._pseudo):
            pseudo._idx = idx

... but this fails because we are following these instructions

        The built-in mechanism to override the list of pseudo positioners on a
        PseudoPositioner is to define '_pseudo' on the class-level.  It should
        be a list of attribute names.

In our test, we setup a Fourc class: https://github.com/bluesky/hklpy/blob/98eb88c0a05dff7b200b58d36bffe77dbdd155cb/hkl/tests/test_extra_motor.py#L13-L27

then subclass per instructions. https://github.com/bluesky/hklpy/blob/98eb88c0a05dff7b200b58d36bffe77dbdd155cb/hkl/tests/test_extra_motor.py#L104-L106 The enumerate(self._pseudo) part misses our additional pseudo-positioner.

Need to refactor to use code that walks through all pseudo-positioners, not just the ones in _pseudo, such as:

            for attr, cpt in cls._sig_attrs.items():
                if issubclass(cpt.cls, PseudoSingle):
                    yield attr, cpt

Originally posted by @prjemian in bluesky/hklpy#48 (comment)

[prjemian]

from hklpy, the Fourc class:

class Fourc(E4CV):
    h = Cpt(PseudoSingle, '')
    k = Cpt(PseudoSingle, '')
    l = Cpt(PseudoSingle, '')

    omega = Cpt(SoftPositioner)
    chi = Cpt(SoftPositioner)
    phi = Cpt(SoftPositioner)
    tth = Cpt(SoftPositioner)

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

        for p in self.real_positioners:
            p._set_position(0)  # give each a starting position

FourcSub subclass:

    class FourcSub(Fourc):
        _pseudo = ['h', 'k', 'l', ]
        p_extra = Cpt(PseudoSingle, '')

[prjemian]

There is no unit test for this feature. The _idx attribute is used to indicate within the position tuple. Additional pseudos are not in this tuple. The position property for a pseudo depends on the _idx so it is obvious this feature is not implemented at this time.

Either the instructions should be changed, removing the instructions shown above, or this will need more work to implement the feature allowing additional pseudos.

[tacaswell]

Why do you want to add an additional pseudo axis that is not included in any of the pseudo machinery? Looking at that code (last touched in 2015 when @klauer originally wrote it), my guess is that the intention of _pseudo was to allow the "promotion" of components that were not PseudoSingnal components, not to hide additional pseudo axis from the machinery.

[prjemian]

It's a valid question and I can only conjure up one case. I was trying out the documented feature for .pseudo the same way it is documented for ._real.

The case I conjure is to compute |hkl| vector magnitude Q as a convenience. This could be done as a property but it does not show up with a diffractometer.read() call. Some diffraction experiments are to scan energy at fixed Q. Others are to scan around a constant Q arc offset from a specific (hkl).

I don't quite understand the promotion of components that are not PseudoSignal. Better that we document ._pseudo with advisory that it is intended for this particular and not for general use since all pseudo positioners are handled as arguments in the .forward() method. Thus they are expected to be known a priori.

Please improve my clumsy wording here.

[tacaswell]

I would like @klauer to weigh in, but I am in favor of ripping that extra control out. If you use it to mask that a PseudoSignal is not actually a pseudo axis it breaks they way you noted in pyhkl. I'm not sure what would happen if told it that a normal Signal was part of the pseudo axis (which if it is a soft signal, not sure why you would want that; if it is a real EpicsSignal I am not sure it would work and if it did you would have made a cross IOC/ophyd object pseudo object and that seems like a Bad Idea).

The case of adding Q is a case that we need to add a one-way mulit-source derived signal (as compared to our current DerivedSignal which can do a 1:1 transform).

There is no use of cls._pseudo in any of the NSLS-II profiles, @klauer @ZLLentz are you using this any place at LCLS?

[ZLLentz]

@tacaswell we have some code that inspects self._pseudo, but we don't modify cls._pseudo anywhere. I didn't even realize this was an option.

[tacaswell]

The class version is also always shadowed by an instance version that is created at init time.

[prjemian]

Note this problem cannot be solved in bluesky/hklpy#48

[tacaswell]

@prjemian Are you OK with the suggestion to remove this functionality (which does not work) rather than trying to fix it?

[prjemian]

Yes.

[tacaswell]

Ok, understanding this again I think what is going on here is:

1. defining a class level _pseudo does work, in that it correctly prevents the parent class from considering one of the PseudoSingles as a a child
2. When the parent discovers it has a pseudo axis it pushes some state into the PseudoSingle instance. This is because the axis need to reflect some shared state from the parent class (computed by forward or inverse I can never remember which one goes which way) so rather than knowing how to compute their values, the PseudoSingles only know how to use the (private) state of their parent to access the correct value on get and invoke the parent with all of the over values the same on put.
3. Because we skipped a component in the set up it does not have the set up (ok, a bit tautological)
4. When we try to read the un-initialized PseudoSingle it tries to use the null-state to reach up to the parent and fails (spectacularly).

I guess we could raise a better error, but I'm not sure what the intended purpose of this feature was.

1. if you want a simple Signal, then just use a Signal
2. if you want a read-only pseudo axis excluding it this way will not get you that (and I guess you could do terrible things and directly set it, but then in than case, it is option 1 and please don't)
3. if you want to mask out a PseudoSingle from a parent class you can set it to None in the class body, however please do not (it means your forward/inverse are completely different and I'm 95% sure sight-unseen that such an inheritance will make your life more confusing later (and if your story involves multiple inheritance that goes double (but you are already in for pain so maybe the marginal cost is lower?)))

So I come to:

1. it "works"
2. it is has no purpose any more.

If we were starting now, I might want to do the pseudo axis only with Type annotation 😈 .