Te-Wei Tsai feel free to ignore the contents of the "fitter" directory, or review it, as you prefer. I added it as the 2nd-to-last commit and asked Bo Xin [X] to review it as part of reviewing DM-26283.

The update looks good! Great job done!

Te-Wei Tsai's review

ts_hexapod

fitter/README.rst

1. How about use the hyperlink ((text)[link]) for the followings two links?
https://github.com/lsst-ts/ts_hexapod/pull/20/files#diff-4724d953195547353c56576dcf07b11cR29-R30
The same for:
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.txt&file-filters%5B%5D=.yaml#diff-708b1e48fa63e6b7142d34ec09e40c11R2

fitter/fit_data.py

1. It might be good to explain the expected data format somewhere:
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.txt&file-filters%5B%5D=.yaml#diff-0264c1c78aa9cc662dccfe0b27dd4c00R76
Or people read the file directly?
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.txt&file-filters%5B%5D=.yaml#diff-708b1e48fa63e6b7142d34ec09e40c11R3-R4

2. Not sure this function is really needed or not:
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.txt&file-filters%5B%5D=.yaml#diff-0264c1c78aa9cc662dccfe0b27dd4c00R100-R101

3. It might be good to provide/print the covariance value to user to have an idea that how good this fitting is:
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.txt&file-filters%5B%5D=.yaml#diff-0264c1c78aa9cc662dccfe0b27dd4c00R150-R152

4. I prefer to put the global variable (plot_dict) as in argument input instead of using it directly. Or you could explain this is a global variable.
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.txt&file-filters%5B%5D=.yaml#diff-0264c1c78aa9cc662dccfe0b27dd4c00R184

5. Do you want to put some argument to fit_data.py to save the image instead of showing it? Some people might use the terminal to use it while the code is deployed in a remote server. I know people can use the image forward. But take myself as an example, I only do this when I try to use the LabVIEW...

6. I am not sure which of zenith or elevation angle will be used in hexapod compensation/ LUT. There is a discussion recently to let the subsystem uses the elevation angle in LUT. Maybe you could discuss this with Bo. He is trying to let people have the consensus now. This is a recent topic in AOS meeting.

compensation.py

1. elevation_coeffs=] --> elevation_coeffs=[
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.yaml#diff-e186c6ccf746831ed5315a1c840f0e33R43-R50
BTW, I do not think other developer except you will understand this format. Is it the polynomials of x, y, z, u, v, and w from up to down? And if you write the [0.31, 0.032, -0.0033] (is this the z-axis?), does it mean something like: "0.31 + 0.032 * x - 0.0033 x^2" or "0.31 * x^2 + 0.032 *x - 0.0033" or else?

2. You forgot the period:

https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.yaml#diff-e186c6ccf746831ed5315a1c840f0e33R56
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.yaml#diff-e186c6ccf746831ed5315a1c840f0e33R58

3. Do you want to check the temperature and azimuth are in the range or not? Although I do not see the input of azimuth is used in this function...
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.yaml#diff-e186c6ccf746831ed5315a1c840f0e33R114-R115

schema/Hexapod.yaml

1. Do we need to add "the" before "first order":
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.yaml#diff-939f6e5067f2f5b959f0164ca76894aeR44
https://github.com/lsst-ts/ts_hexapod/pull/20/files?file-filters%5B%5D=.py&file-filters%5B%5D=.yaml#diff-939f6e5067f2f5b959f0164ca76894aeR49

ts_config_mttcs:

default.yaml

1. It might be good to add some description like this: This configuration file is mainly for the compensation used in the hexapod's movement. The coefficients here are used in the hexapod's compensation or LUT. The available range of elevation or temperature is XXXX. For the "up to down" and "left to right", the order in the fitting is XXXX. If the polynomial fit is used, the unit of coefficient is XXXX. If the Fourier series is used, the unit is XXXX. If the cosine polynomial is used, the unit of coefficient is XXXX.

This might give the user has some understanding of this file's details. But this might not be needed as well. BTW, for the order, I mean:

    - [0]

    - [6.41754532e+02, 5.52880420e-01, -2.06081416e-01, 1.14135938e-04, 3.68377168e-06]

    - [-6.05001038e+02, 1.58266699e+01, 4.76907398e-03, -1.00806469e-03, 2.63260121e-06]

    - [1.77291772e-02, -2.02560397e-07, -5.40226646e-06, 1.20390387e-11, 1.37038647e-10, 2.79592334e-15, -1.44466393e-15, 5.14073174e-19, 5.26607158e-21]

    - [0]

    - [0]

I should say if I do not read the code to know the implementation details, I will not understand what these numbers mean.

Thank you for another very helpful review. Here are my responses.

ts_hexapod

fitter/README.rst

1. The links should already show up as hyperlinks (should anyone render the file in html). The form you recommend allows the link text to be shorter, but in this case I did not shorter text would be much use, though I suppose it could highlight Camera vs M2.

fitter/fit_data.py

1. Good point. I put a much clearer reference to the README in fit_data.py. I am keen to not duplicate the description of the data format.

2. The function return_one simplifies the implementation of the fourier function by making the code more regular. Every coefficient is treated the same. I tried implementations that treat C0 separately, but felt the additional code needed to handle the special case was unpleasant.

3. A good point. I will make it optional, since I think it is often clutter.

4. Thank you for the excellent suggestion. In fact fit_all_axes is the only function that requires the plots, so I changed the code to create the figure and plots in fit_all_axes.

5. Thank you for the excellent suggestion. I added the --graphpath argument which, if specified, saves the plot and quits without displaying it.

6. The Hexapod uses elevation: elevation is the argument to moveWithCompensation command, and that was also true before this ticket: elevation was the argument in the previous moveLUT command. In addition, telescope motion uses elevation, not zenith distance and the pointing component accepts elevation, not zenith distance when commanding an el/az target. Thus I would be very resistent to changing the Hexapod to use zenith distance.

The fitter accepts zenith distance in order to handle the existing FEA and possible future compensation data to be fit, should the collector of the data prefer zenith distance for some reason.

compensation.py

1. Good catch. Fixed.

2. Fixed.

3. There are no limits on range for azimuth and temperature. I added documentation to that effect to Compensation.get_offsets.

schema/Hexapod.yaml

1. OK.

default.yaml

1. In my opinion all information about the format of config files and the meaning of the values in them belongs in the schema. This avoids dangerous duplication of information.

In particular, this applies to your final remark. Users must read the schema in order to understand the config files. If the descriptions there are inadequate then that is a problem. I certainly tried to make the format and meaning of the coefficients clear in the description fields of the schema.

(That said, I do think config files should contain comments explaining why the particular values are chosen. So in this case I provided the source of the data that was fit.)

In your note you mention the fourier and cosine polynomial models, but neither of those is supported in Hexapod configuration files.

For the record: I hope that someday I will be allowed to switch to Fourier for elevation, and to add it for azimuth. I do not expect us to ever use the cosine polynomial. I provide support for it in the fitter for historical reasons.

Merged to develop. I will wait for the release of ts_xml 6.3 to release new versions of ts_hexrotcomm, ts_hexapod and ts_rotator.