Replace hard-wired DFT functions with IDFT #136

brettviren · 2021-11-23T21:29:21Z

This PR is on top of #135 and will stay draft until that one is merged. I also still need to do more testing on this one.

This removes and replaces the hard-wired DFT functions from Waveform and Array used in various components and tests with methods called through an IDFT.

This can only be an improvement as the garbage return value was being ignored by all callers.

Tests still needed.

brettviren · 2021-11-24T18:38:36Z

Some mid-PR notes:

The above commits replace use of the old hard-wired DFT functions from Waveform.h and Array.h with calls that go through an IDFT instance. The old functions and their call sites have been commented out but remain as temporary place holders to help make sure the transliteration to the new call patterns is correct. After testing, they will be removed.

Here is a source file level list of current IDFT users.

❯ grep '\["dft"\]' */src/*.cxx|sed -e 's/:.*//g'
gen/src/AddCoherentNoise.cxx
gen/src/AddNoise.cxx
gen/src/DepoTransform.cxx
gen/src/EmpiricalNoiseModel.cxx
gen/src/Misconfigure.cxx
gen/src/NoiseSource.cxx
gen/src/PerChannelVariation.cxx
gen/src/PlaneImpactResponse.cxx
gen/src/TruthSmearer.cxx
sig/src/Decon2DFilter.cxx
sig/src/Decon2DResponse.cxx
sigproc/src/L1SPFilter.cxx
sigproc/src/Microboone.cxx
sigproc/src/Microboone.cxx
sigproc/src/OmnibusSigProc.cxx
sigproc/src/OmniChannelNoiseDB.cxx
sigproc/src/Protodune.cxx
sigproc/src/Protodune.cxx
sigproc/src/SimpleChannelNoiseDB.cxx

Some of these source files define more than one component.

These are the types of changes to the configuration which are needed and will come in subsequent commits:

Get the default FftwDFT config object into the configuration sequence. This must be done even if we never touch individual component config. OTOH, this is automatic when we we do that touching (via the use lists).
For any existing configuration of components in the above list, have a way to set their dft config parameter.
Provide a global spot to set a non-default dft that then makes it into these per-component settings.

… code into the one and only subclass

…nding the problem

brettviren · 2021-12-03T16:33:42Z

That last commit added an IDFT config to all locations I could find.

A few caveats and comments.

There is a really distressing amount of copy-paste that has crept into our configuration. I think we must wait for PR #105 to be ready and merged before coming up with a serious rewrite.

The tools object holds the default IDFT (FftwDFT). This object can be overridden to use a non-default IDFT.

local default_tools = tools_maker(params)
local tools = std.mergePatch(default_tools, {
  dft: {type: "TorchDFT", data: {device: "gpu"}}});

However, the PlaneImpactResponse is itself created in tools and thus there is no way to provide a non-default IDFT. Fixing this must wait until the config rewrite.

Top level cfg/l1sp/ and cfg/uboone/ directories are seen. These are not "standard" and I do not know their purpose so I have ignored them. If they are not used they should be deleted. If used, they likely are now broken w.r.t IDFT.

Here is a summary of the files changed:

brettviren · 2021-12-03T18:04:34Z

Some additional info

Besides the unit tests passing, this high-level test gives reasonable looking results:

  wire-cell -l stdout -L debug \
      -c test/test-pdsp-sim-sp-dnnroi.jsonnet

  wirecell-plot ntier-frames -o foo.pdf \
    test-pdsp-ssd-orig0.tar.bz2 \
    test-pdsp-ssd-wiener0-gauss0.tar.bz2 \
    test-pdsp-ssd-dnnsp0.tar.bz2

There is also a correctness tester:

  wirecell-aux run-idft

Note, this sometimes "fails" due to round-off errors slightly greater than 1e-6. And, run it from the top of wire-cell-toolkit/ directory so it can find the build/aux/check_idft program.

And there is a benchmark program.

It runs with FftwDFT by default:

  wirecell-aux run-idft-bench -o fftw-cpu.json \
    build/aux/check_idft_bench

or Torch/CPU:

  wirecell-aux run-idft-bench -o torch-cpu.json \
    -p WireCellPytorch -t TorchDFT \
    build/aux/check_idft_bench

or Torch/GPU:

  wirecell-aux run-idft-bench -o torch-gpu.json \
    -p WireCellPytorch -t TorchDFT \
    -c aux/test/test_idft_pytorch.jsonnet \
    build/aux/check_idft_bench

and then make plots from the *.json files:

  wirecell-aux plot-idft-bench -o idft-bench.pdf \
    fftw-cpu.json torch-cpu.json torch-gpu.json

HaiwangYu

Need to comment out this line due to removal of gen/inc/WireCellGen/ImpactZipper.h

HaiwangYu · 2021-12-08T19:30:57Z

pytorch/src/DFT.cxx

+#include "WireCellUtil/NamedFactory.h"
+
+#include <torch/script.h>
+#include <torch/csrc/api/include/torch/fft.h>


seems this file is not available for older libtorch versions, e.g. 1.6 which is the latest version on gpvm.

Hmm, this takes me by surprise.

Fermilab should upgrade. IIRC, they also do not build libtorch with CUDA support which also limits their usefulness.

I am inclined to say that we simply do not include WCT's pytorch sub-package for the wirecell UPS product until they upgrade. To avoid a build-time error in the case that libtorch is found automatically would rquire adding --with-libtorch=no to wcb configure. My own intention is to use this new torch support in a stand-alone build of WCT so I don't care about this UPS breakage.

An innocent bystander of that choice would be that dnnroi will also not be available in a UPS environment until Fermilab upgrades. If we "hide" the libtorch based DFT behind some #ifdef then at least dnnroi could run on CPU at Fermilab. But, I don't know if doing that is on any immediate goal (is it?).

What's your opinion on either of these approaches, @HaiwangYu ? Any other solution?

Seems this fft.h is available from 1.7. https://github.com/pytorch/pytorch/tree/v1.6.0/torch/csrc/api/include/torch
And the versioning macros are available from 1.8 https://pytorch.org/cppdocs/notes/versioning.html
I think we can skip the pytorch folder until gpvm has a newer version. Before that, if DNNROI is needed, we can remove DFT.cxx source file in the building shims. How about this? @brettviren

Sounds very good to me. Thanks.

My build was picking up a stale copy in the install dir

HaiwangYu · 2021-12-08T21:30:09Z

cfg/pgrapher/common/sim/nodes.jsonnet

            }
-        }, nin=1, nout=1),
+        }, nin=1, nout=1, uses[tools.dft]),


uses=[tools.dft]

HaiwangYu · 2021-12-08T21:31:43Z

cfg/pgrapher/common/tools.jsonnet

+    // The IDFT FFT implementation 
+    dft : {
+        type: "FftwDFT",
+    }


HaiwangYu · 2021-12-10T19:57:56Z

I did some tests for Vertical Drift 3view30 with default FftwDFT seems the results are consistent with current master
pr136.pdf
.

brettviren added 27 commits November 1, 2021 16:12

Initial draft of a DFT interface and FFTW implementation

c7ba58f

Tell Boost to shut up with the internal deprecation warnings

59be25c

Tell Boost to shut up with the internal deprecation warnings

a9787bb

Add stack trace to exception what()

08815ba

Throw instead of returning garbage on garbage input.

a7b6f97

This can only be an improvement as the garbage return value was being ignored by all callers.

Start on higher-level dft functions

dd4dffb

Allow to optionally provide data and metadata in constructor

dbcbe9b

More work toward DFT as a service

2ffbb84

Elaborate on comments

c248bab

Flesh out and test DftTools interface

cd3369d

Add axis, transpose, tests

a83cba8

Improve tests

e8e0625

More testing

28d9ec9

Make a semaphore interface and implement with what is in util

c798cd4

Move common code to a 'context' mix-in, add initial torch imp of IDFT

fc8dc58

Work out brain bugs in understanding torch tensor storage

22d5ea0

Typo in plugin name fixed

47e67dd

Initial draft of an IDFT benchmarker

d90bdcc

Improve timing measurements

a46fd9a

Rename, too slow to run each time

3c65ffb

Make this more globally accessible so test/check programs can use it

365c21c

Improve benchmark

359b50f

Fix regression in cli arg parsing

bbe851d

Remove obsolete 'zipper' based sim

0a49ef6

Pass IDFT::pointer by const ref

2bedce6

Port gen to IDFT

ee42c72

First draft removal of hard-wired DFT in favor of IDFT.

c499a53

Tests still needed.

brettviren added 2 commits November 29, 2021 12:27

Fix lack of storing device name.

927d8ef

Fix the ignored base class config methods by removing/moving the base…

fedd276

… code into the one and only subclass

brettviren added 12 commits November 29, 2021 12:29

Measure first exec+plan separate from subsequent execs

28af349

Add single precision complex float

3411d42

A start on a correctness test for IDFT

f31dcaa

Add r2c and c2r helpers

b774679

Replace cli11 with boost po and get checker fleshed out

e89f0c9

Fix row/col-wise monkeys

df869f3

More eigen row/col-wise testing

8160700

Fix one wrong conversion to IDFT, improve logging along the way to fi…

db16d54

…nding the problem

Remove noisy

07d8a3d

Fix typo in dispatch

a9e7eb8

Allow passing dft config down

5cf456b

More passing down of IDFT

f4f02c4

brettviren mentioned this pull request Dec 2, 2021

DFT interface and implementations #135

Closed

Add DFT throughout config.

f581274

brettviren added 2 commits December 3, 2021 11:51

Remove previously commented-out old-style dft()/idft() calls

1f282d5

Add construction of IDFT for tests

2c13e3c

brettviren marked this pull request as ready for review December 3, 2021 17:31

brettviren requested a review from HaiwangYu December 3, 2021 17:32

HaiwangYu reviewed Dec 8, 2021

View reviewed changes

HaiwangYu self-requested a review December 8, 2021 19:37

Remove include of removed header

d8ddb6e

My build was picking up a stale copy in the install dir

HaiwangYu reviewed Dec 8, 2021

View reviewed changes

cfg/pgrapher/common/sim/nodes.jsonnet Outdated

}

}, nin=1, nout=1),

}, nin=1, nout=1, uses[tools.dft]),

Copy link

Member

HaiwangYu Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uses=[tools.dft]

HaiwangYu reviewed Dec 8, 2021

View reviewed changes

cfg/pgrapher/common/tools.jsonnet Outdated

// The IDFT FFT implementation

dft : {

type: "FftwDFT",

}

Copy link

Member

HaiwangYu Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},

Few fixes found by Haiwang in review

847fb83

HaiwangYu approved these changes Dec 10, 2021

View reviewed changes

brettviren merged commit 79ca54e into master Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace hard-wired DFT functions with IDFT #136

Replace hard-wired DFT functions with IDFT #136

brettviren commented Nov 23, 2021

brettviren commented Nov 24, 2021

brettviren commented Dec 3, 2021

brettviren commented Dec 3, 2021

HaiwangYu left a comment

HaiwangYu Dec 8, 2021

brettviren Dec 8, 2021

HaiwangYu Dec 9, 2021

brettviren Dec 9, 2021

HaiwangYu Dec 8, 2021

HaiwangYu Dec 8, 2021

HaiwangYu commented Dec 10, 2021

Replace hard-wired DFT functions with IDFT #136

Replace hard-wired DFT functions with IDFT #136

Conversation

brettviren commented Nov 23, 2021

brettviren commented Nov 24, 2021

brettviren commented Dec 3, 2021

brettviren commented Dec 3, 2021

HaiwangYu left a comment

Choose a reason for hiding this comment

HaiwangYu Dec 8, 2021

Choose a reason for hiding this comment

brettviren Dec 8, 2021

Choose a reason for hiding this comment

HaiwangYu Dec 9, 2021

Choose a reason for hiding this comment

brettviren Dec 9, 2021

Choose a reason for hiding this comment

HaiwangYu Dec 8, 2021

Choose a reason for hiding this comment

HaiwangYu Dec 8, 2021

Choose a reason for hiding this comment

HaiwangYu commented Dec 10, 2021