Project

General

Profile

Feature #23343

thread-safe replacement for LArFFT

Added by Michael Wang about 1 year ago. Updated about 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
09/26/2019
Due date:
% Done:

100%

Estimated time:
4.00 h
Spent time:
Experiment:
-
Co-Assignees:
Duration:

Description

This is a thread-safe replacement for the LArFFT service. The name of the feature branch is mw-larfftw. Note that it is named "LArFFTW" so it does not interfere with the continued use of the LArFFT service. This is not a service since it need not be one. It also does not require the libfftw3f_threads library in order to support thread-safe operation. It does this by creating a re-usable plan (done through LArFFTWPlan which is protected by a mutex) and using the "New Array Execute Functions" of fftw3 so that the same plan can be re-used on new input data. This has been tested successfully on prototype multithreaded versions of deconvolution modules using DUNE and ICARUS samples. This has also been presented at a LArSoft coordination meeting and the last LArSoft workshop.

History

#1 Updated by Lynn Garren about 1 year ago

This is a request to merge feature/mw-larfftw into lardata. However, a quick glance shows that the code has grown a dependency on larreco. This would create a circular dependency. If the larreco dependency is necessary, then the code will need to move into larreco (presumably anyway).

#2 Updated by Michael Wang about 1 year ago

the code it is dependent on is the marquardt fitter which was also authored by me. I created this fitter originally for the non-root based gauss hit finder we developed in the scidac-hepreco project. Using it in LArFFTW allowed me to completely eliminate any dependency on root.

#3 Updated by Kyle Knoepfel about 1 year ago

  • Estimated time set to 4.00 h
  • Assignee set to Kyle Knoepfel
  • Status changed from New to Assigned
  • Co-Assignees Michael Wang added

#4 Updated by Tracy Usher about 1 year ago

I suppose I'm not sure I can remember why one needs to fit a gaussian in LArFFT... but the really important question is when can this issue get resolved so we can try moving forward with the thread safe implementation?

ICARUS is going to need to be trying to run parallel as much as possible and within 1-2 months max.

#5 Updated by Lynn Garren about 1 year ago

  • Assignee changed from Kyle Knoepfel to Lynn Garren

LArFFTW is designed to replace the complete functionality of LArFFT. LArFFT uses a root fitter. LArFFTW uses the new MarqFitAlg fitter, which depends only on lardataobj. Mike has talked with Giuseppe and they have agreed that the fitter can be moved to lardata. I will take a look at moving it. We will need a simple migration script for user code.

#6 Updated by Tracy Usher about 1 year ago

I wonder if LArFFT is trying to do too much?

In essense, what is needed is a thread safe "service" for providing access to a Fast Fourier Transform and its inverse. But it looks to me that LArFFT is really written as a set of tools for doing convolution and deconvolution as part of signal processing (since the tools it defines appear to be used in SignalShapingServices, also in lardata/utilities and which is a base class for experiment specific signal processing - except experiments using wire cell and icarus which stopped using all of that).

For ICARUS we had stopped using LArFFT in most places (and as I find it I replace it so eventually all places). I had been replacing with the eigen FFT interface which, as least on my Mac, uses FFTW (I'd been led to believe that was theadsafe). Since the eigen interface is in an "unsupported" folder it is probably better to not use it going forward, but at least it gives access to an FFT without all the other unnecessary items.

Any chance we can either make LArFFT a cleaner interface (and move the other stuff to another service or integrate into the signal processing code)?

#7 Updated by Kyle Knoepfel about 1 year ago

Tracy, are you making a comment on the LArFFT service, which has been in LArSoft a long time, or the LArFFTW class that Mike has put together and is the subject of this feature?

#8 Updated by Tracy Usher about 1 year ago

I think to the extent that LArFFTW is meant to replicate/replace LArFFT then I guess both. I searched around a bit, the function "PeakCorrelation" defined in LArFFT(W) which performs a gaussian peak fit is called by the function "SetPeakResponseTime" in SignalShapingServices. In turn, this function is not called internally by SignalShapingServices, and as near as I can tell this it is not called by DUNE, MicroBooNE and/or ICARUS (I did not check other experiments). I would postulate this s yet another bit of "leftover" code.

So, I'm suggesting that it might be worth considering whether this "old" code might be pruned away rather than moving an algorithm used directly in LArReco to an external package.

Alternatively, why not simply replicate the Eigen interface to the FFTW package directly? Or just use that directly? I believe I was told that FFTW is thread safe... what I think is the issue is that directly interfacing to it is not "obvious" to the casual user (e.g. me). The Eigen interface is straightforward and seems to work well. As I understand from Mike, on linux the Eigen interface appears to use a different, non-thread safe, underlying FFT package but I'd imagine this could be corrected easily.

I say all this because a few years ago I had been given the impression that it was better to not use the LArFFT service as a general FFT interface (I think because it has to reinitialize for different use cases)

I'm not religious about this, if expediency says move the algorithm and move on then fine. But it looks to me to be an opportunity to do some possible code cleanup so worth considering.

#9 Updated by Michael Wang about 1 year ago

I had replaced the root fitter in PeakCorrelation with the marquardt fitter because it was trivial to do so. I don't have strong feelings about keeping PeakCorrelation there. If we can confirm it is not being used or will ever be used by anyone, I'm fine with removing it if it simplifies things. On the other hand, I think the standalone marquardt fitter is a simple and useful enough tool that it would be a good idea to have it in lardata utilities instead of larreco.

I think that Lynn is preparing or has prepared a migration script. If this is close to being done, I would just push through with the requested changes and we can always remove/prune unused things like PeakCorrelation later.

My suspicion about eigen using a backend different from fftw was based on this link: https://eigen.tuxfamily.org/index.php?title=EigenFFT#kissfft which claimed that kissfft is the default backend. I never really confirmed this on linux as Tracy had on the mac -- finding that eigen interfaces to fftw. It could be the information on the eigen page is not up to date and it is actually alos using fftw on linux. If

Eigen does appear to employ a very simple interface to fftw. Unfortunately I don't understand how it interfaces with fftw, including how it creates the fftw plan -- which is the major non-thread-safe part of fftw. From their pages, eigen appears to be thread-safe but I recall that when I tried running art/larsoft in multithreaded mode, the eigen-based icarus modules would crash. LArFFTW may not provide as simple a user interface but we know exactly how it intefaces to fftw, because we implemented it ourselves. In our goal of using fftw in a thread-safe way, for example in processing many wires in parallel, I think it is a big advantage to know exactly how we are calling the underlying fftw libraries.

#10 Updated by Lynn Garren about 1 year ago

Everything is in place for the next release of larsoft. We are simply waiting for a non-related fix before we can tag.

#11 Updated by Lynn Garren about 1 year ago

  • % Done changed from 0 to 100
  • Status changed from Assigned to Resolved

LArFFTW is now available in the larsoft v08_32_00 release.

MarqFitAlg was moved from larreco to lardata/Utilities. Since we found no use of MarqFitAlg outside larreco and LArFFTW, a migration script was not needed.

#12 Updated by Lynn Garren about 1 year ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF