6.1.3 was from the middle of the 2021-11 run, 6.2.0 includes transform changes which were were running (and thus had tested) at the end of the run, plus a couple of tickets from after the run.

If I am not being dim there is a bug in the new MeasureDistortion.clip() but have no idea whether it could show your problem. If I put a fix for that on a branch can you test?

That bug fix did not fix Chi-Hung's problem. Leaving branch open, and leaving the .clip() change on the branch.

So, I did two tests last night. Two phi-arm motor map runs. The result looks like the following figure with version 6.1.3.

Those spots are exactly at expected positions. However, if I use version 6.2.0, they looks lie this.

The spots are everywhere!

Chi-Hung will provide Hassan and Yuki input data to test the transform routines in pfs_utils and help to identify the problem.

Chi-Hung to confirm that 6.1.4 and 6.1.5 work correctly.

Uploading notebooks file for transformation INSTRM-1588.ipynb

OK, now I have better picture. Please see the plot. This evening, I use MCS with different versions of pfs_utils to transform cobra locations from pixel to mm unit. Since I did not command cobra to move, so they are pretty close to the cobra center (red spot). As you can see, 6.1.2 and 6.1.3 return reliable transformations.

The plots above shows that tag 6.1.4 is not reliable, but this tag is not applied to any code on any branch in GitHub, so possibly a fork: https://github.com/Subaru-PFS/pfs_utils/commit/9d076ba123d0933baafd009bd0682944f740eaff .

If you have the opportunity, then a test with tag 6.1.5 would be great.

(My apologies. I wrote an earlier comment following a ICS/PFI telecon discussion suggesting to check 6.1.4, but I did not check at that time that this tag is in fact invalid).

6.1.4 is the code which was being used at the end of the Nov run. It was at the tip of the INSTRM-1462 branch, which shortly after got merged to master and tagged 6.1.5, but after a rebase on top of a branch which introduced a sky-pfi transform change (INSTRM-1491).

6.1.4 was left at the end of that branch for posterity. We could add a more descriptive label name, or even a branch name if the GitHub web interface is confused.

So checking against both 6.1.4 and 6.1.5 would be interesting. If 6.1.4 does not work I am worried, since it contained the last few RHL transform changes, some of which mattered quite a bit.

According to Chi-Hung's latest plots, 6.1.4 is problematic, as well as 6.2.0 which seems to give different results to 6.1.4.

Yuki and I independently find a bug in the sigma-clipping algorithm which was introduced after 6.1.3 . Preparing a fix. However I see Craig's comment above where this appears to be discounted. cloomis: can you elaborate why your fix did not work?

Fix pushed. chyan could you test please?

chyan ran with the change, but still saw the problem. So the fix is still on the branch, but it does not solve this problem.

There was also a swap in two of the distort() args (scale and theta), which seemed far more plausible, but I never tracked down how that could have leaked out into mcs/fps/cobracharmer.

The recent proposed fixes are on a git branch named "PIPE2D-1588". Given the time I will go ahead and rename that to "INSTRM-1588"

Boring FYI: my fix on INSTRM-1588 was subsumed by Hassan's, so I removed my old branch and renamed Hassan's to INSTRM-1588.

hassan Please check the result of your fix. The transformation is still not good.

chyan Can you provide me with the frameId related to the MCS pixel coordinates you used to perform the transformation in the above plot please?

chyan I think you should call pfa_utils by setting insr0t=0 anytime for 71M camera.

By changing

pt = transformUtils.fromCameraName('rmod',altitude=90, insrot=teleInfo['insrot'].values[0])

to

pt = transformUtils.fromCameraName('rmod',altitude=90, insrot=0.)

(and using the same parameters as before) I got the same result. (without this change, I see a few micron difference, which I thought acceptable yesterday...)

Pushed the latest fix (previously in u/hassans/20220511a) to the ticket branch, such that it is ready to be merged to master.

This fix re-introduces the basic algorithm in the MeasureDistortion._call_ method that was used in the pfs_utils 6.1.3, ie., without sigma-clipping.

I believe that the sigma-clipping algorithm is however still valid, but a little more work is required to check its robustness across a number of scenarios. Given that there is little time before the May Eng Run, this should be addressed in a separate ticket after that run.

The change isn't in the sigma clipping, it's in the definition of the cost function (np.mean(d**n); n = 2 works; n = 1 doesn't).

With n=1 the initial solution looks like this. . Note that the same number of ring fiducials are matched with both the initial and second match (but different fiducials). With n=2 the two matches are to the same fiducials, and the final fit is good. In other words, while n=2 works it's basically due to good luck.

The solution is to add a prior on the rotation for the original ring fit; this seems to work well.

Here is the solution with initial cost function of np.sum(d) + alphaRot*theta**2 with alphaRot = 1 for the "ring" fit. Note also the change from np.mean to np.sum; this means that when we have lots of points the Likelihood should dominate the prior on theta.

The code sets alphaRot=0 for the final solution, so there's no bias due this prior (it'd be small anyway for any reasonable alphaRot as the number of points fitted is much larger).

I have just pushed these changes to the ticket branch.  We need to make sure that chyan sees them and makes suitable changes in the calling code to take advantage of this work.

One comment: if the algorithm (n=1) selected different fiducials in the first and second match, due to rotational symmetry. Is it good practice in any case of a prior, for Chi-Hung to select the original fiducials such that it is rotationally asymmetric? Eg pick 5 fiducials on the left of the PFI and 3 on the right?

I have tested Robert's fix. Attached plot show that the current transformation.

The new fix consistent with the center positions of cobra. I would suggest this ticket to be closed and merge to mater.

Merged RHL's fix to master.