You'll probably want to rewrite this in python if the C++ code doesn't just work. We'll need to handle blends at some point – remember that we know the wavelengths (very?) well.

I have not started on this ticket yet, but do you mean there is an existing code in C++?

One simple question is whether this method would use LSF or not.

There is C++ code, but it dates from Andreas and I don't think it's easy to extend.  It fits a Gaussian, which is good enough for isolated lines but probably/possibly not for blends.

I have a python implementation of finding and fitting lines as part of PIPE2D-319.

I'm sorry for the delayed response but I have looked at Paul's implementation and it looks good enough.

Should we merge this tickets to PIPE2D-319?

I have basically copied Paul's code to python/pfs/drp/stella/SpectrumContinued.py.

naoki.yasuda can you add your updates to the ticket branch for this ticket please? tickets/PIPE2D-345.

naoki.yasuda could you create a pull request so that Paul and I can review the code before it is merged to master? Thanks.

I suggest extending the new FindLinesTask to return the intensities in addition to the centroids.

Following 2D DRP tech telecon 2019-10-14: re-assigned to price . Suggested algorithm makes use of gaussians, which may not be adequate for broadened arc lines. However, if the broadened arc lines are limited (ncaplar feels that there should not be more than 5 of such lines) we could remove those lines from the modelling process.

I have put a proposed implementation in u/price/PIPE2D-345 of drp_stella (GitHub PR). It's currently on a u branch to avoid contaminating the original proposal.

naoki.yasuda, would you please have a look and let me know if this does not meet your needs?

There are changes made as part of PIPE2D-391 that move the line fitting into C++ for speed. This will cause some conflicts with the work done here, but should be straightforward to resolve.

Thank you. I have reviewed the changes of the code. I think it is fine and very useful for my task. One thing I'm not sure but not so important is that in a function FindLinesTask.convolve() there is a line

        grow = int(self.config.width + 0.5)

Why width is used instead of halfSize?

We don't want to grow the masked pixels by the kernel size, because pixels at the edge of the kernel have only a tiny contribution to the convolved value. For the LSST image convolutions, we grow by a single pixel, but maybe that's not ideal, so I've made this configurable.

I put your work onto u/yasuda/PIPE2D-345 and moved my work from u/price/PIPE2D-345 onto the formal ticket branch, tickets/PIPE2D-345.

naoki.yasuda, are you too busy to do a code review, or should I ask someone else?

What is the official procedure of code review? In the previous comment, I meant I have reviewed and the code looks fine. Is this not enough?

Ah, I thought perhaps you had only considered the idea, and not the actual code. I'll go ahead and merge. Thanks!

Merged to master, and tagged 5.1.4.