JeffersonLab / analyzer

HallA C++ Analyzer
BSD 3-Clause "New" or "Revised" License
8 stars 53 forks source link

being able to rapidly turn on decoder debugging #147

Closed rwmichaels closed 6 years ago

rwmichaels commented 6 years ago

When a problem happens with the decoder, which unfortunately can happen, it's useful to be able to turn on all the debug statements -- for example below. This way one can immediately see if all the modules are getting called and receiving their data, etc. I tried this naively from the analyzer script using analyzer->GetDecoder()->SetDebugFile("decode.txt"); analyzer->GetDecoder()->SetDebug(1);

It's a natural thing to try, but doesn't work. (Error: illegal pointer to class object GetDecoder())

Next I made a workaround that did this in the analyzer script analyzer->SetVerbosity(4); and then I have to add some code to THaAnalyzer.

We may discuss it and I can produce a solution.

Here is an example of the debug printout I am talking about. It is in Caen1190Module.C if (fDebugFile != 0) fDebugFile << "Caen1190Module:: 1190 MEASURED DATA >> data = " << hex << p << " >> channel = " << dec << tdc_data.chan << " >> raw time = " << tdc_data.raw << " >> status = " << tdc_data.status << endl;

hansenjo commented 6 years ago

GetDecoder() returns a null pointer until you've done Init().

The simplest solution is this: In your script, do

analyzer->Init(run);

then do

analyzer->GetDecoder()->SetSomething(somevalue);

and continue with

analyzer->Process(run);

We can change THaAnalyzer to instantiate the decoder from the constructor, so an explicit Init() won't be necessary for what you want to do. That would work fine until someone decides to override the decoder class with THaInterface::SetDecoder. In that case, the object returned by THaAnalyzer::GetDecoder() before Init() would not be the actual decoder used, which could lead to confusing behavior and bugs.

Additionally, we can read decoder parameters from an (optional) database file. Like all database files, this one would be read at Init() time. The behavior would be clear, but setting debug parameters in a file instead of the script is a less attractive solution.

Opinions, anyone?

rwmichaels commented 6 years ago

I think your simplest solution may be best; however, one may want the GetDecoder() method to warn "Hey, you need to analyzer->Init(run)" if it detects that problem. Then a user is less clueless. I suppose, however, I could have figured this out eventually from studying the code.

So, I'll try your simplest solution and see if I can implement a user-friendly warning, too.

I suppose that would work even if someone did THaInterface::SetDecoder, right ? (Which hardly anyone would do, except maybe doing monte carlo ...?)

Bob

----- Original Message ----- From: "Ole Hansen" notifications@github.com To: "JeffersonLab/analyzer" analyzer@noreply.github.com Cc: "Robert Michaels" rom@jlab.org, "Author" author@noreply.github.com Sent: Wednesday, November 8, 2017 1:09:37 PM Subject: Re: [JeffersonLab/analyzer] being able to rapidly turn on decoder debugging (#147)

GetDecoder() returns a null pointer until you've done Init().

The simplest solution is this: In your script, do

analyzer->Init(run);

then call GetDecoder()->SetSomething(somevalue), then continue with

analyzer->Process(run);

We can change THaAnalyzer to instantiate the decoder from the constructor, so an explicit Init() won't be necessary for what you want to do. That would work fine until someone decides to override the decoder class with THaInterface::SetDecoder. In that case, the object returned by THaAnalyzer::GetDecoder() before Init() would not be the actual decoder used, which could lead to confusing behavior and bugs.

Additionally, we can read decoder parameters from an (optional) database file. Like all database files, this one would be read at Init() time. The behavior would be clear, but setting debug parameters in a file instead of the script is a less attractive solution.

Opinions, anyone?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_JeffersonLab_analyzer_issues_147-23issuecomment-2D342904759&d=DwICaQ&c=lz9TcOasaINaaC3U7FbMev2lsutwpI4--09aP8Lu18s&r=18qXn_9GgntmnnlhbPFXjg&m=_jlgfUNVmzy580qoa0p-fBUQFi1HSMXz9d1-pqozXls&s=VwYygQP9u9HvwhjlXTvpS5O6de5QFIirRj7Nh_hhPDc&e=

hansenjo commented 6 years ago

Sure, adding a warning/hint is very easy. If fEvdata is null when GetDecoder() is called, print the message. A form of built-in documentation. Git and other tools have that too as of late, very helpful indeed.

You're right, SetDecoder() is meant for Monte Carlo input, although one could also use it for reading other (non-EVIO) input formats.

After Init(), GetDecoder() always returns the actual decoder object to be used, which is of either the default decoder class (Decoder::CodaDecoder) or whatever class the user set with SetDecoder(). So setting decoder parameters after Init() will do the right thing, regardless of whether SetDecoder() was called or not.

hansenjo commented 6 years ago

As discussed, added the warning message to THaAnalyzer::GetDecoder (commit 924b9efe64a73db6ddb4be070f4e02eb3a7d551d). Closing this issue as this was largely a documentation problem.