Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call wirecell deconvolution from dataprep #23

Open
dladams opened this issue Mar 7, 2023 · 15 comments
Open

Call wirecell deconvolution from dataprep #23

dladams opened this issue Mar 7, 2023 · 15 comments
Assignees

Comments

@dladams
Copy link
Contributor

dladams commented Mar 7, 2023

The current DUNE reco strategy runs a dataprep module which writes full (i.e. no ROI filtering) processed waveforms as recob:Wire to the event data store which are used as input to the WCT (wirecell) module which does deconvolution and ROI finding (and more?) writing another recob::Wire container to be used in hit finding. There are two problems with this approach:

  1. The intermediate container is left in the event store taking up memory only to be eventually discarded
  2. There is no opportunity to run dataprep tools on the deconvoluted data to explore additional noise removal, run alternate ROI finders, generate metric plots such as noise vs. channel, or make dataprep event displays.

Point 2 could be addressed by running another dataprep module after WCT without ROI finding but that implies yet another recob::Wire entry in the event data store and even another if we utlimately decide to then run another WCT module to do the ROI finding. A second approach would be to somehow insert dataprep tools in the WCT processing chain but this looks difficult as WCT has a private notion of tools and configuration. The subject of this issue is a third approach where WCT sequences are wrapped as dataprep tools so they can be called in the dataprep processing chain. If successful, it will then be possible to run the full signal processing chain (raw data to ROIs suitable for hit finding ) in a dataprep module with options to take algorithms for decovolution and ROI algorithms from WCT or from other sources.

@dladams dladams self-assigned this Mar 7, 2023
@dladams
Copy link
Contributor Author

dladams commented Mar 7, 2023

The first step here is to demonstrate it is possible to create a dataprep tool that runs a WCT algorithm. On that subject, I had some email exchanges with Brett and edited summaries follow. From me after poking into the WCT code:

I found 
 virtual bool operator()(const boost::any& anyin, boost::any& anyout) = 0
in IFunctionNode —> IFrameFilter but I don’t see the connection to OmnibusSigProc.
Does that provide this operator()?

Responses from Brett:

   Link against libWireCellSigProc.so and create a concrete instance of
   OmnibusSigProc.  On that instance, you will need to call the methods
   defined by IConfigurable and IFrameFilter.

and:

Call default_configuration(), alter what it returns as desired and pass
to configure().

Then call operator()(const IFrame::pointer& in, IFrame::pointer& out).

-Brett.

I intend to take this as a starting point.

@brettviren
Copy link
Member

The proper thing is to develop a WCT flow graph node that runs dataprep code.

Your current strategy will defeat DUNE's ability to benefit from multi-threading in this critical data and computationally intensive processing stage. Unless, that is, you are willing to reinvent all the MT wheels that WCT already provides.

Alternatively, DUNE can use WCT's existing Noise Filter node, instead of dataprep, which does not introduce these new problems and which has been tested on PDSP data.

@dladams
Copy link
Contributor Author

dladams commented Mar 7, 2023

Brett: Let's have that discussion in a forum outside this ticket. I would like to use this as a place to track progress toward the goal above. Briefly, we appear not to agree on the proper way to proceed and I believe the most useful place to use MT is within the tool/algorithms. Thanks.

@brettviren
Copy link
Member

We just had a two day workshop showing how that is not a good strategy.

@tomjunk
Copy link
Member

tomjunk commented Mar 7, 2023

It sounds like there is a problem with support for certain features in art that make a simple fcl solution to this problem not effective in terms of memory conservation. A solution that currently is clumsy but ought not to be would be to define instances of dataprep and wirecell that work only on one APA at a time. We can do this now, by restricting processing in our current chain to one APA, and replicate the trigger path entries that run dataprep and then wirecell with appropriate configurations.

The problems with this are:

  1. As David says, this does not solve the memory problem -- the intermediate recob::Wire data products sent from each dataprep instance to the corresponding WireCell instance still clutter up memory. And post-WireCell producing of even newer versions of recob::Wire will consume further memory

  2. Initialization of multiple instances of WireCell will take memory and CPU. And may not even work (maybe?), though most art modules are written so that they can be multiply instantiated.

Problem #1 can be solved if we can remove produced products from the event. Currently this throws an exception. Kyle has been open to removing this exception. It is meant to protect sloppy programmers, but any time there is a memory deallocation feature provided in code, it is possible to shoot ones self in the foot and we should accept that possibility. I suppose we could add an optional argument with a default value to removeCachedProduct() so that the default behavior remains for all existing code. We would then write simple deleter modules to clean up memory no longer in use. It would be good to remove cached products anyway as it provides freedom to use art's event memory for temporary data, make a plot, save it, etc.

Another feature we have asked for but not gotten is "eager writing". If we were interested in writing these cooked waveforms out to a file but not clutter memory with them we cannot use art's ROOT I/O to do that. Kyle asked at the workshop for what priority to assign to flushing data out to output files before the event processing is finished.

Another feature I am not sure really exists in art yet is module-level parallelism. The schedule parallelism in art seems tied to event processing, and communication with Kyle and others seems to indicate that if we want sub-event parallelism we'd have to use TBB ourselves.

WireCell allows us to do all of these things but we must code within it and do our own file i/o on output for example. Since WireCell is further down the link stack from dataprep and is also experiment agnostic, one would have to define plug-ins and callbacks to call the dataprep tools from within WireCell.

There are lots of benefits to using art's ROOT I/O (single output file, config info, provenance, etc). But we cannot yet stream output to it.

@dladams
Copy link
Contributor Author

dladams commented Mar 8, 2023

Tom:

I agree that we want the option to do the signal processing (raw data to ROIs at least) one APA or CRU at a time. Even if we can remove recob:Wire objects from the event data store, it does not make sense to create these objects and is preferable to pass them in the transient format used by dataprep or wirecell. I would like to be able to make multiple handoffs, e.g. noise removal in dataprep, deconvolution in wirecell and then ROI finding in dataprep. More complicated sequences including metric evaluation and event display should also be supported.

Avoiding recob::Wire conversions and event data store I/O is best accomplished by running a single module that gets input from a raw data input tool, runs a sequence of algorithms and writes the recob:Wire objects to be used as in put to hit finding. Either dataprep or wirecell can provide the module with its own algorithms. Here I am proposing to investigate enabling the former to additionally call the algorithms from latter. I will likely next work on the reverse problem: running dataprep algorithms in the wirecell module.

I expect that once we have single APA/CRU processing we will fit in 2 GB and there will no longer need the complexity of having multiple threads processing an APA and can run with a single (computational) thread. If this is not the case, then we will likely want something like TBB threads within each algorithm because processing a new APA just adds to the memory load. This then implies simultaneous processing of multiple APAs and events to try to keep the additional threads busy. It still remains interesting to offload some of the processing (e.g. FFTs) to GPUs but I expect this will be most useful within algorithms.

We are then left with difficult choice of running the dataprep module or wirecell module which has up to now avoided with ugly choice of running both. There are strong arguments for each module though naturally I prefer the first. I don't want to have that discussion here. I believe this a difficult problem that the SW architecture group should take up.

@dladams
Copy link
Contributor Author

dladams commented Mar 9, 2023

I am trying to instantiate OmibusSigProc but get this error

Consolidate compiler generated dependencies of target dunedataprep_DataPrep_Service_ThresholdNoiseRemovalService_service
/home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:7:10: fatal error: WireCellSigProc/OmnibusSigProc.h: No such file or directory
    7 | #include "WireCellSigProc/OmnibusSigProc.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

I looked in the code for example but only found an include for the OmnibusSigProc source file:

(base) mac> grep -r OmnibusSigProc wire-cell-toolkit/ larwirecell/ | grep include
wire-cell-toolkit//sigproc/src/OmnibusSigProc.cxx:#include "WireCellSigProc/OmnibusSigProc.h"
(base) mac> 

@dladams
Copy link
Contributor Author

dladams commented Mar 23, 2023

I presume the above error comes because dunedataprep does not have dependency on wire-cell-toolkit. I tried adding larwirecell to ups/product_deps but that alone did not help. I then additionally added this line to the top-level CMakeLists.txt:

find_ups_product( wirecell )

and instead of the above error, I saw:

[ 26%] Building CXX object dunedataprep/dunedataprep/DataPrep/WctTool/test/CMakeFiles/test_wirecell.dir/test_wirecell.cxx.o
In file included from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellAux/Logger.h:44,
                 from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellSigProc/OmnibusSigProc.h:4,
                 from /home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:7:
/cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellUtil/Logging.h:14:10: fatal error: spdlog/spdlog.h: No such file or directory
   14 | #include "spdlog/spdlog.h"
      |          ^~~~~~~~~~~~~~~~~
compilation terminated.

I added spdlog in the cmake file and then got this error:

[ 26%] Building CXX object dunedataprep/dunedataprep/DataPrep/Service/CMakeFiles/dunedataprep_DataPrep_Service_InterpolatingAdcMitigationService_service.dir/InterpolatingAdcMitigationService_service.cc.o
In file included from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellUtil/Point.h:5,
                 from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellUtil/Pimpos.h:5,
                 from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellIface/IWirePlane.h:12,
                 from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellIface/IFrame.h:5,
                 from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellIface/IFrameFilter.h:6,
                 from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellSigProc/OmnibusSigProc.h:6,
                 from /home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:7:
/cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellUtil/Configuration.h:4:10: fatal error: json/json.h: No such file or directory
    4 | #include <json/json.h>
      |          ^~~~~~~~~~~~~
compilation terminated.

Then I added the jsoncpp product (json didn't work) and got to this:

In file included from /cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellSigProc/OmnibusSigProc.h:13,
                 from /home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:7:
/cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellUtil/Array.h:30:10: fatal error: Eigen/Core: No such file or directory
   30 | #include <Eigen/Core>
      |          ^~~~~~~~~~~~
compilation terminated.

Adding eigen didn't fix this but when I also added this to the local cmake file:

include_directories(${EIGEN3_INCLUDE_DIR})

I did get past this and can see syntax errors in my code.

I put back the original product_deps file and get the same result:

[ 26%] Building CXX object dunedataprep/dunedataprep/DataPrep/Service/CMakeFiles/dunedataprep_DataPrep_Service_DuneDPhase3x1x1NoiseRemovalService_service.dir/DuneDPhase3x1x1NoiseRemovalService_service.cc.o
/home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:22:7: error: ‘fhicl’ has not been declared
   22 | using fhicl::ParameterSet;
      |       ^~~~~
/home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx: In function ‘int test_wirecell()’:
/home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:37:3: error: ‘OmnibusSigProc’ was not declared in this scope; did you mean ‘WireCell::SigProc::OmnibusSigProc’?
   37 |   OmnibusSigProc osp;
      |   ^~~~~~~~~~~~~~
      |   WireCell::SigProc::OmnibusSigProc
In file included from /home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:7:
/cvmfs/larsoft.opensciencegrid.org/products/wirecell/v0_23_0/Linux64bit+3.10-2.17-e20-prof/include/WireCellSigProc/OmnibusSigProc.h:22:15: note: ‘WireCell::SigProc::OmnibusSigProc’ declared here
   22 |         class OmnibusSigProc : public Aux::Logger,
      |               ^~~~~~~~~~~~~~
/home/dladams/proc/build/dev01/workdir/srcs/dunedataprep/dunedataprep/DataPrep/WctTool/test/test_wirecell.cxx:40:11: error: ‘osp’ was not declared in this scope
   40 |   cout << osp.default_configuration() << endl;
      |           ^~~

This is progress.

@dladams
Copy link
Contributor Author

dladams commented Mar 24, 2023

I fixed the compile-time errors and now have linker problems:

[ 28%] Linking CXX executable ../../../../slf7.x86_64.e20.prof/bin/test_wirecell
/usr/bin/ld: cannot find -lWireCellSigProc
collect2: error: ld returned 1 exit status

Again cribbing from larwirecell, I added this to the top-level cmake file:

cet_find_library( WIRECELL_SIGPROC_LIB NAMES WireCellSigProc PATHS ENV WIRECELL_LIB NO_DEFAULT_PATH )

That alone did not solve the problem and so I added wirecell to product_deps:

[dladams@jupyter-dladams ups]$ git diff product_deps
diff --git a/ups/product_deps b/ups/product_deps
index 4f9f965..9dd94f9 100644
--- a/ups/product_deps
+++ b/ups/product_deps
@@ -25,16 +25,17 @@ defaultqual e20
 product          version
 cetbuildtools   v8_20_00       -       only_for_build
 dunecore         v09_69_01d00
+wirecell         v0_23_0
 end_product_list
 
 
 # We now define allowed qualifiers and the corresponding qualifiers for the depdencies.
 # Make a table by adding columns before "notes".
-qualifier       dunecore           notes
-c7:debug        c7:debug     
-c7:prof         c7:prof      
-e20:debug       e20:debug    
-e20:prof        e20:prof     
+qualifier       dunecore     wirecell           notes
+c7:debug        c7:debug     c7:debug
+c7:prof         c7:prof      c7:prof
+e20:debug       e20:debug    e20:debug
+e20:prof        e20:prof     e20:prof
 end_qualifier_list
 
 # Preserve tabs and formatting in emacs and vi / vim:

I chose version v0_23_0 because I got this error when I tried the one I copied from larwirecell:

Error encountered when setting up product: wirecell
ERROR: Version conflict -- dependency tree requires versions conflicting with current setup of product wirecell: version v0_7_0a vs v0_23_0
ERROR: setup -B wirecell v0_7_0a -q e20:+prof failed
ERROR: setup of required products has failed

But, after all this, I still get the missing library error. I finally fixed that by changing the library reference in the local cmake file from WireCellSigProc to WIRECELL_SIGPROC_LIB. Now I get json library errors.

I see jsoncpp references I can copy from larwirecell but now I wonder if I would be better off adding explicit dependency on that package instead of wirecell in product_deps and the cmake file. Any thoughts @brettviren or @tomjunk ?

@dladams
Copy link
Contributor Author

dladams commented Mar 24, 2023

From the beginning, I have added larwirecell to product_deps, added this to top-level cmake file:

[dladams@jupyter-dladams test]$ git diff ../../../../CMakeLists.txt
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7978698..e90c7b3 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -41,6 +41,11 @@ find_ups_product( larcorealg )
 find_ups_product( lardata )
 find_ups_product( lardataalg )
 find_ups_product( lardataobj )
+find_ups_product( lawirecell )
+find_ups_product( wirecell )
+find_ups_product( jsoncpp )
+find_ups_product( spdlog )
+find_ups_product( eigen )
 
 include(MakeDuneToolBuilder)
 include(dunecore::ToolTypes) # Enable simpler building of tools.

and this in WctTools/CMakeLists.txt

cet_find_library( WIRECELL_SIGPROC_LIB NAMES WireCellSigProc PATHS ENV WIRECELL_LIB NO_DEFAULT_PATH )
cet_find_library( WIRECELL_IFACE_LIB NAMES WireCellIface PATHS ENV WIRECELL_LIB NO_DEFAULT_PATH )
cet_find_library( WIRECELL_UTIL_LIB NAMES WireCellUtil PATHS ENV WIRECELL_LIB NO_DEFAULT_PATH )
cet_find_library( WIRECELL_APPS_LIB NAMES WireCellApps PATHS ENV WIRECELL_LIB NO_DEFAULT_PATH )
cet_find_library( JSONCPP_LIBS NAMES jsoncpp PATHS ENV JSONCPP_LIB NO_DEFAULT_PATH )
set(WIRECELL_LIBS ${WIRECELL_APPS_LIB} ${WIRECELL_SIGPROC_LIB} ${WIRECELL_IFACE_LIB} ${WIRECELL_UTIL_LIB} ${JSON_CPP_LIBS})

I can now compile and link and get this result from my test program:

dune-dev> workdir/build_slf7.x86_64/dunedataprep/slf7.x86_64.e20.prof/bin/test_wirecell 
test_wirecell: -----------------------------
test_wirecell: Instantiate OmnibusSigProc.
test_wirecell: Display default config.
{
        "ADC_mV" : 2047999999.9999998,
        "anode" : "AnodePlane",
        "break_roi_loop1_tag" : "break_roi_1st",
        "break_roi_loop2_tag" : "break_roi_2nd",
        "charge_ch_offset" : 10000,
        "cleanup_roi_tag" : "cleanup_roi",
        "ctoffset" : -8000,
        "decon_charge_tag" : "decon_charge",
        "dft" : "FftwDFT",
        "elecresponse" : "ColdElec",
        "extend_roi_tag" : "extend_roi",
        "fft_flag" : 0,
        "field_response" : "FieldResponse",
        "frame_tag" : "sigproc",
        "ftoffset" : 0,
        "gain" : 2.2430470818000003e-12,
        "gauss_tag" : "gauss",
        "inter_gain" : 1.2,
        "isWarped" : false,
        "loose_lf_tag" : "loose_lf",
        "lroi_jump_one_bin" : 0,
        "lroi_max_th" : 10000,
        "lroi_rebin" : 6,
        "lroi_short_length" : 3,
        "lroi_th_factor" : 3.5,
        "lroi_th_factor1" : 0.69999999999999996,
        "mp2_roi_tag" : "mp2_roi",
        "mp3_roi_tag" : "mp3_roi",
        "per_chan_resp" : "PerChannelResponse",
        "r_break_roi_loop" : 2,
        "r_fake_signal_high_th" : 1000,
        "r_fake_signal_high_th_ind_factor" : 1,
        "r_fake_signal_low_th" : 500,
        "r_fake_signal_low_th_ind_factor" : 1,
        "r_low_peak_sep_threshold_pre" : 1200,
        "r_max_npeaks" : 200,
        "r_pad" : 5,
        "r_sep_peak" : 6,
        "r_sigma" : 2,
        "r_th_factor" : 3,
        "r_th_peak" : 3,
        "r_th_precent" : 0.10000000000000001,
        "rebase_nbins" : 200,
        "shaping" : 2200,
        "shrink_roi_tag" : "shrink_roi",
        "sparse" : false,
        "tight_lf_tag" : "tight_lf",
        "troi_asy" : 0.10000000149011612,
        "troi_col_th_factor" : 5,
        "troi_ind_th_factor" : 3,
        "troi_pad" : 5,
        "use_multi_plane_protection" : false,
        "use_roi_debug_mode" : false,
        "use_roi_refinement" : true,
        "verbose" : 0,
        "wiener_tag" : "wiener",
        "wiener_threshold_tag" : "threshold"
}
test_wirecell: -----------------------------
test_wirecell: Done.

dladams added a commit that referenced this issue Mar 24, 2023
@dladams
Copy link
Contributor Author

dladams commented Mar 24, 2023

I pushed my changes to develop. I first had to pull recent changes including this odd package tag: v09_69_01g01 which I left in produt_deps.

@dladams
Copy link
Contributor Author

dladams commented Apr 3, 2023

Poling around in wirecell toolkit, it looks like SimpleFrame is the concrete IFrame subclass that I should use to pass data to and retrieve data from wirecell. Later I might look at making my own class with the IFrame interface wrapped around dataprep structures.

@dladams
Copy link
Contributor Author

dladams commented Apr 4, 2023

I am creating a utility DpWctFrameConverter that is constructed from an AdcChannelDataMap and provides methods to extract or set its waveforms to or from an IFrame object.

@dladams
Copy link
Contributor Author

dladams commented Apr 4, 2023

@brettviren I see that IFrame holds a interger identifier "ident". Is that simply geometry identifier or does it indicate the stage of processing? Most important, when we apply a transformation, will the output IFrame have the ident as the input? Thanks. --da

@dladams
Copy link
Contributor Author

dladams commented Apr 18, 2023

After some time off for school breaks, I am back working on the dataprep-wirecell frame converter. I at least want to get it to build so I can push the code to develop. After updating header and source, I can nowI can now compile. To get things to link, I added many missing inline directives in AdcChannelData.h and added find_library directives for the wirecell aux library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants