Feature/sm sur

[AldenJurling]

This branch adds support for SM moves to the move_sur() method. Because segment moves are made through move_seg_local and SM moves are made through move_sm_local, this requires a check in move_sur on whether the segment is, in fact, the SM, and take a different code path if it is.

In the course of this I also implemented the group= keyword argument which was previously ignored. The behaviour was unspecified so I made it take a zero-based index into the groups and execute that one.

I also added support for moves in meters because the SUR in order to process the GA SUR I was working with it.

I can rebase this branch off of your current master or latest merge commit if you prefer a cleaner history.

I'm not sure why my webbpsf master branch picked up the 2018-12 base commit its branched from.

[coveralls]

Pull Request Test Coverage Report for Build 151

• 0 of 9 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.06%) to 40.812%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
webbpsf/opds.py	0	9	0.0%

Totals
Change from base Build 146:	-0.06%
Covered Lines:	2041
Relevant Lines:	5001

---

💛 - Coveralls

[mperrin]

It looks like the update.absolute parameter is not passed through to the SM move command. I'm guessing this is an unintentional omission? I can't remember the details but I assume that absolute moves are allowed for the SM just as for any primary segment.

[mperrin]

Looked through the code and this seems good to me. I did find one missing function parameter that could be passed through to the SM move command, but that's minor. Let me know if you agree with adding that and I can make that change directly.

In re-reading this code I regret that the SM is handled in a separate function from all the PMSAs; it would've been more elegant to wrap those complexities into the same function so the same API could be used, just treating the SM as segment 19. Could refactor that way at some point but it's probably not a priority right now.

[mperrin]

@AldenJurling - wanted to ping you re: my comments on this PR from several weeks ago. Could you take a moment to look at those and let me know how you want to proceed, please? Should only take a minute or two. Thanks.

[AldenJurling]

@mperrin - I admit I don't remember exactly what I was thinking about absolute moves, but looking at the code, absolute doesn't appear to be a valid argument to move_sm_local. I'm not sure if I missed it, or just didn't pass it through since it wasn't a legal argument.

    def move_sm_local(self, xtilt=0.0, ytilt=0.0, rot_unit='urad',
                      xtrans=0.0, ytrans=0.0, piston=0.0, trans_unit='micron', display=False,
                      delay_update=False):
        """ Move the secondary mirror in pose, using segment-local control coordinates.

        These motions are always commanded in the segment-local "Control" coordinate systems,
        which are distinct for each segment. The SM is also handled a bit differently than all the
        PMSAs in terms of its local coordinate system, which this function handles behind the scenes.

        Parameters
        -----------
        segment : str
            Segment name, e.g. 'A1'. Use 'SM' for the secondary mirror.
        xtilt, ytilt, clocking : floats
            Tilt angle, in microradians by default. 'xtilt' means tilt *around the X axis*, and similarly for ytilt.
        xtrans, ytrans, piston : floats
            Displacement distance, in microns by default.
        trans_unit : str
            Unit for translations. Can be 'micron', 'millimeter','nanometer', 'mm', 'nm', 'um'
        rot_unit : str
            Unit for rotations. Can be 'urad', 'radian', 'milliradian', 'arcsec', 'arcmin', 'milliarcsec'
        display : bool
            Display after moving?
        delay_update : bool
            hold off on computing the WFE change? This is useful for computational efficiency if you're
            moving a whole bunch of segments at once. Incompatible with display=True.
        """

I may have just assumed it wasn't valid for the SM since it was supported downstream, but I don't have any reason to know that. If its a valid option in the SUR, which it probably is, it needs to be implemented in move_sm_local and then threaded through to it. That should be pretty easy, but I won't be able to do it right now.