pcbend / GRUTinizer

Let's grutinize!
8 stars 32 forks source link

Support for CAGRA (type 14 GEB format) & (optionally) Grand Raiden at RCNP #110

Open csullivan opened 8 years ago

csullivan commented 8 years ago

This merge request brings in support for parsing type 14 GEB event data (Digital Gammasphere), which is useful for analyzing data from multiple systems, including the Clover Array Gamma-ray spectrometer at RCNP/RIBF -- CAGRA. It also brings in the necessary TDetector systems and event source for coupling GRUTinizer with the RCNP unpacker utilized with the Grand Raiden (GR) spectrometer and the Large Acceptance Spectrometer (LAS) (TRCNPSource library). This latter addition is optional and can be enabled by checking out the GRAnalyzer submodule from https://github.com/CAGRA-GrandRaiden/.

For the purpose of this merge request the GRAnalyzer submodule is not necessary and all of the supporting GRUTinizer libraries are only compiled if RCNP=1 is present in the makefile.

Please test with your normal workflow. I will address any issues if they rise.

csullivan commented 8 years ago

I will also mention that the code was production tested during a recent DAQ test experiment in June at RCNP. This was the main reason for delaying the merge request until now (and hence why it is so large).

Lunderberg commented 8 years ago

A couple comments, and requests for changes before it gets merged in.

csullivan commented 8 years ago

Briefly skimming over things, the suggestions seem reasonable. I will address each point with a commit or comment if further discussion is needed. Thanks!

csullivan commented 8 years ago

Alright, below I respond to your comments if I have not created a commit in response to one (or multiple) of your points. If some action was needed and taken, then I have pushed a commit (see the above).

What is the difference between a pedestal and a baseline? If there is no conceptual difference, we should have them be the same value.

The only difference is that the Asymptotic baseline is rate dependant and therefore it’s convenient to have it be organized by timestamp. I could update the pedestal value so that it can vary with time, but this is not typically what I would expect from a “pedestal”. I’m open to suggestions.

The -G option seems to be unnecessary. We should be able to tell whether or not the argument is a bash-style wildcard without needing to explicitly telling to program.

This flag is used for creating a TGlobRaw source, which is a wrapper on TMultiRaw that checks the globbed directories for new files that may appear after the start of a session. Unless I am misunderstanding, I think it makes sense to have this functionality be utilized via a flag. I noticed just now that in a later comment you mentioned having some unpublished source including the glob source in TGRUTInt. Perhaps this will clarify the issue.

The RCNP_BLD enum type looks to be specific to grand raiden data. Let's change the name to reflect that.

In fact, the RCNP_BLD type is the data type for two separate spectrometers, Grand Raiden (GR), and the Large Acceptance Spectrometer (LAS). It is however a single DAQ. I can rename this to GR_BLD if you prefer, but in principle if RCNP continues to use GRUTinizer, RCNP_BLD is more consistent with their software for GR and LAS.

In the kDetectorSystems, ANL is given a value of 14. When used in the TUnpackingLoop::HandleGEBData, these values and names should correspond to the official Berkeley designations for GEB-type data. Is this value of 14 official, and what is it's official name?

The official Berkley name is DGS for digital gammasphere, however ANL has developed the firmware of the LBL digitizers to work as general purpose digitizers. In this case, I submit that the Berkeley designation needs to be updated since ANL’s use case has evolved beyond DGS and I don’t expect them to change the firmware for each of their detector systems.

In the TRawBanks.h, the various Argonne classes should refer to the detector in question, rather than the lab.

These raw structs are independent of a given detector system, but instead are characteristic of the utilized ANL digitizer firmware. TANLEvent is the primary unpacker class that utilizes these structs. Thus I do not believe it would be advantageous to rename these for a specific detector system.

Could we take another look at packing the TGrandRaiden* into the smart buffer? Wanting to take another look at it to see if we can avoid modifying the TRawEvent class with the extra void*. With the calls to the grand raiden unpacker, can those be made inside TGrandRaiden::BuildHits()?

We can do so, but a word of warning. I have spent many hours debugging a double free that occurs at the destruction of a smartbuffer (in a TRawEvent). There is a possible race condition that seems to occur. For now, I would like to leave the void* in, until we work through this together. If I was arguing for the void*, I would say that it does allow a user to utilize the sorting components of GRUTinizer without requiring them to do the packing / unpacking. I could see this being advantageous in some cases.

The TTreeSource is inside the #ifdef RCNP. With this being something specific to RCNP, it should have a name that reflects this. If it is to be made more general, then it should have the ability to retrieve multiple detector branches from a single root file. In addition, there is a hard-coded path on line 64 of TTreeSource.h that will break outside of nscl-land.

I have wiped this out see commit 965fd8401afdfde50c8feb975cd6744883836d0c.

Does the TANLEvent contain multiple individual events, each with a separate timestamp? This is the reason why the TGEBMode3Event, on which it looks like the TANLEvent is based, is part of the event-structure rather than being part of a detector's unpacking. If each TANLEvent has only a single event, then this should be moved into unpacked, rather than building. Also, the name should be changed to indicate the detector system that uses it.

TANLEvent is analogous to TDDASEvent, not TGEBMode3Event. TANLEvent is a general purpose unpacking class which unpacks the GEBArgonne firmware data according to the type specified in the GEBArgonneHead struct. For this reason and those already discussed, I do not believe that TANLEvent should be modified.

In TGRUTint.cxx, line 152, the if(!opt->TreeSource()) should be unnecessary. The OpenRootFile will only add it to gChain if there exists a TTree named EventTree in it. Is this the case for the grand-raiden-preprocessed root files?

No longer applicable, TTreeSource has been removed.

For the handling of the glob source in TGRUTint::SetupPipeline, I think I have some un-pushed code that handles it without the explicit flag. I'll get back to you on that one.

See above discussion on TGlobRaw.

For the separate thread reading the Grand Raiden events from the ttree, why not just have it be an instance of TStoppableThread? This would remove the necessity for the stop_rcnp_signal variable in TStoppableThread.cxx.

TTreeSource has been removed. The stop_rcnp_signal is used to stop the asynchronous thread that is running the RCNP/GR analyzer in TRCNPSource. The RCNP/GR analyzer maintains it’s own internal loop which cannot be exported out into GRUTinizer without significant changes to the RCNP code base. Since I am trying to ensure a level of maintainability between both repositories, this is the most balanced solution I have found.

In TRawEvent::GetSize(), is there a reason why RCNP_BLD returns sizeof(void*)?

For measuring the throughput, only a pointer is being passed through the GRUTinizer sorting pipeline for each RCNP_BLD event. This is because the first level of unpacking of the RCNP BLD data is happening before it makes it into GRUTinizer. GRUTinizer is then only responsible for time correlating the RCNPEvents with the GEB events. Rather than storing the RCNPEvent class into a TSmartBuffer and then unpacking back into the same structure in TGrandRaiden (which would incur an enormous amount of copies), I simply pass a void* pointer through GRUTinizer which can be cast out into it’s original type once it land in it’s respective TDetector system.

In TRawEventSource::EventSource(), anything with a .root filename is assumed to have RCNPEvents in it. Is there some way we can generalize this?

This has been removed as TTreeSource is ejected.