We discussed this at the 2017-01-09 meeting. The existing tickets/PIPE2D-138 branch contains a single giant commit with a lot of unrelated work. We agreed that @aritter will (in priority order):

• Pull out just the work related to FiberId identification and handle it on this ticket;
• File other tickets to describe the purely mechanical work (adding line lists, etc) which are currently included in the giant commit, and send them out for review separately;
• File another ticket to describe the overall strategy for wavelength calibration at LAM, and provide a description there of the approach which is going to be taken. We will review this at a future meeting.

I changed `findITrace` to return the number of the closest predicted FiberTrace. This function will
now always return a positive number, so I also changed the return type from int to size_t and the return type of `assignITrace` from bool to void.

Per discussion of 2017-01-18,

• Point 1 above has been done, and is now on this ticket.
• Points 2 & 3 have not been addressed.

Points 2 & 3 to be attacked on PIPE2D-148.

The single giant commit which was on this ticket branch is now on branch PFSMeeting: https://github.com/Subaru-PFS/drp_stella/tree/PFSMeeting in separate commits (but Andreas assures me we are not losing anything).

swinbank, if you haven't started reviewing the ticket yet, I would have 1 more commit to add which I found stashed away, replacing one number with a control parameter

Fine. I'll set the ticket back to "in progress"; please let me know (by setting the ticket to "in review") when your additional commit is added.

In addition to the comments on GitHub:

• I don't see tests. Specifically, I'd like to see tests which demonstrate that each individual function does what you expect and an overall integration test. You should be able to test (for example) findITrace with a variety of different inputs and demonstrate that it always returns the correct trace.
• 9072604e4bee80fe8b6ebf488ce9557144ff011e is broken: it garbles the argument list of assignITrace. You then fix it in a later commit, but that's not good enough: the code should compile after each atomic change.
• 9072604e4bee80fe8b6ebf488ce9557144ff011e has a misleading commit message: it refers to yCenter, but the argument is actually yCenters.
• Why are c08dcb427617691d2a47cb84a54c83e3937b16e5 and baf1012421a0ca1f9922487b359cb7e6668b9a9d separate commits? If you are changing the return type to reflect changes in the logic of the function, why aren't you changing the return type at the same time as you change the logic?
• The formatting of the commit message in 2f20a2ff6fa1f1a9868b3a8420e7d0760c3de260 is incorrect (there's a weird extra new line).
• In 2f20a2ff6fa1f1a9868b3a8420e7d0760c3de260 you add the maxDistance parameter. However, I can't find anywhere that that parameter is actually used! I'm not sure whether that's because there's something more subtle here than I understand, or whether you forgot. Can you clarify, please? (And as per the first point above: this functionality should be tested, so I can see that it works by just checking that the test runs.)

Mostly quite minor comments, here and on GitHub. However, the lack of tests is a big problem, and I'd like to understand properly how maxDistance is being used.

swinbank, I just found out that the fiber IDs in RedFiberPixels.fits.gz contain gaps what requires a code change in order to get the correct fiber IDs. I would shortly move this ticket back to 'In Progress' and fix that unless you have any objections...

According to cloomis the slit has more positions than there are science fibers, and the
pipelines will only see the science fibers.
The missing fiber IDs are gaps (between the two CCDs, say), fiducial fibers, alignment fibers, etc.
It was decided to number all fibers according to the slit position, and not, say, the science fiber index.

That's from me, and I figure I should state the convention: fibers are numbered according to their position along the slit, and not according to their index as visible science fibers. Besides the science fibers which the pipeline sees, there are gaps between fiber bundles and between the two CCDs, and there are other fibers – fiducial alignments fibers, etc.

So all of PFS numbers fibers from 1 to 651, even though there are only ~600 science fibers per spectrograph.

I think you mean [-324, 324]

I thought that fibres start at 1. What did cloomis mean?

I've set this back to "in review" — as suggested by @aritter above — until the above issues are resolved.

cloomis meant what he said, but 1) left out the 1-indexing and 2) got the numbers wrong. Yes, the fibers should be numbered from 1 to 651, and the science fibers span 2 to 650

The file which aritter has is not correct, as it is still indexed w.r.t. the center being 0.

I didn't check that the code is correct.

Please look at the review comments and deal with as appropriate. Sometimes this means filling a ticket.

Merged into master