Skip to content

Conversation

@csullivan
Copy link

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.

Chris Sullivan added 30 commits February 26, 2016 13:37
mode 3 data from the LBL digitizers with the Argonne
National Lab firmware. This will allow us to instrument
DGS and CAGRA data in GRUTinizer.
parser should now handle the Argonne National Lab data with firmware v1.1
and of the LED type. Support for other formats and versions will come later.
Added ANL as a detector system, and am now ready to adjust GRUTinizer
unpacking logic to handle events from raw data files.
and added TArgonne/ANL to the TUnpackingLoop,
and TUnpackedEvent case-switch statements to
build TArgonne event parer into GRUTinizer. I
have also added a check for the raw ANL event
file extension gtd* to TGRUTOptions. Finally,
I added endian swaps into the BuildHits.
Ready for testing.
Added MakeANLhistos.cxx for analysis. chicka, chi-kaaah!
Notes: Added support for the ANL firmware LBL digitizers,
including firmware v1.1 for Digital Gammasphere, and CAGRA.
Support for additional ANL firmware versions is upcoming.
…nly BuildHits is passed the raw_data reference

instead of TDetector storing internally
…ntry, creating a NEW event in memory each time.

A pointer to this location in memory is serialized into the TSmartBuffer of the TRawEvent, which is passed on. GetEntry
fills fEvent which is the type over which TTreeSource is specialized. For now I added a prototype EventType class to demo
it's usage and how to register it in the LinkDef. This will be deleted after testing with data has been done.
…ctionality to

TGRUTint::ApplyOptions and TRawEventSource::EventSource. Needs testing.
…e source.

Additionally, updated makefile to link against the external libRCNPTREE.so
Some issue exists in the root file not being picked up as a TTreeSource.
…randRaiden hits where unpacking can be done.

Time to test event correlation between CAGRA and GR!
having TArgonne and TArgonneHit, I refactored TArgonneHit into a TANLEvent type
that is responsible for doing the unpacking, and then the other classes are
solely responsible for building the unpacked event into calibrated events.
TArgonne/TArgonneHits have been renamed to TCAGRA/TCAGRAHits. Left to do:
Need figure out why Clover channel 0 is not appearing in histos. Need to
test to see if the positions map is working right. Need to test if the
calibrations file is working right. See if the segmentation is working or
if the TCAGRASegmentHit class is overkill. Finally, need expand channel
calibrations to include second order corrections such as asymptotic baseline
and pole-zero corrections, for example.
…tions now seem to be working.

Each clover detector gets a number from 01 - 16, and each clover has A,B,C,D leaf crystals. Each crystal can have segments or not.
The naming scheme works as follows: CLO01AP00 this indicates the system is CLO short for Clover. The next number 01 indicates this
is clover 01. A indicates the crystal number (should be the crystal leaf at smaller theta). P indicates that the signal is positive
and 00 means that this is the 0th segment (ie the central contact). Some detectors will have multiple segments in which case this
last number will be 01 or 02. The address is currently made from the BoardID and Channel number. So Board 113 (0x71) and channel 01
would be address 0x01007101. The first 01 is just for historical reasons, and indicates the subsystem number.
Merging RCNPSource branch maintained in CAGRA+GR organization
TCagra::BuildHits to distinguish between ArraySubpositions (Leaves of a clover). Now fixed.
Chris Sullivan added 17 commits July 18, 2016 11:07
… in main code body.

Instead, they are mostly found in source files pertaining to RCNP analysis routines which rely on externel git submodules.
…I notice a race condition when terminating the input thread.

See here for backtrace during exit: https://gist.github.com/csullivan/fd9431b459630ef0f76e70a3f961fb29
…thread, to avoid

a deadlock owing to an incomplete push after the GRUTinizer threads have shutdown. Also changed the behavior of
the RCNP stdout so that it only prints status to screen in offline mode (ie without gui).
Large merge from upstream (pcbend) as well as a few additions from collaboration repo
… RCNP libGRAnalyzer.so and associated TDetector systems.
Preparing large merge with upstream (pcbend)
@csullivan
Copy link
Author

csullivan commented Aug 5, 2016

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
Copy link
Collaborator

Lunderberg commented Aug 8, 2016

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

  • TCaesar.h was modified, adding the <functional> header. Is this change needed?
  • Name-wise, TArgonne is a bit unspecific. Currently, each folder in TDetector is named by the detector that it unpacks. Could it be renamed to the particular detector system being unpacked for consistency?
  • The std::runtime_error being thrown by TChannel::PoleZeroCorrection. Currently, if a value is not present, we either return a default value or nan if there is no reasonable default value.
  • 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 -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.
  • Is the -T option still necessary? I remember that at some point you were able to read grand raiden data from a raw binary file. Is this still the case?
  • The RCNP_BLD enum type looks to be specific to grand raiden data. Let's change the name to reflect that.
  • 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?
  • In the kFileType enum, there is the ROOT_DATA value. This seems to be specific to Grand Raiden, not generic input data from a root file, so the name should reflect that.
  • In the TRawBanks.h, the various Argonne classes should refer to the detector in question, rather than the lab.
  • 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()?
  • 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.
  • 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.
  • 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?
  • 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.
  • 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.
  • In TRawEvent::GetSize(), is there a reason why RCNP_BLD returns sizeof(void*)?
  • In TRawEventSource::EventSource(), anything with a .root filename is assumed to have RCNPEvents in it. Is there some way we can generalize this?
  • Since the target libGRAnalyzer does not produce a file named libGRAnalysis, it should be listed as a .PHONY target.
  • In the _load_default_style(), we would like style.SetOptStat() to remain uncommented. These are going to be moved to the .grutrc at some point anyways to be configurable. Similarly, we would like for the SetPadTick commands not to be present.

@csullivan
Copy link
Author

csullivan commented Aug 8, 2016

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

Chris Sullivan added 6 commits August 16, 2016 09:43
> TCaesar.h was modified, adding the header. Is this change needed?
Removed change to TCaeser.h that is no longer necessary due to the ifdef guard.
> Name-wise, TArgonne is a bit unspecific. Currently, each folder in TDetector is named by the detector that it unpacks. Could it be renamed to the particular detector system being unpacked for consistency?
I've changed it to TCagra. Note however that TANLEvent.cxx (which is generalized to different ANL detector systems) resides in the now renamed TCagra directory.
> The std::runtime_error being thrown by TChannel::PoleZeroCorrection. Currently, if a value is not present, we either return a default value or nan if there is no reasonable default value.
I now return a default value with a warning that is supressed after 5 consecutive calls.
> Is the -T option still necessary? I remember that at some point you were able to read grand raiden data from a raw binary file. Is this still the case?
I have removed TTreeSource and the corresponding flag and front end logic within TGRUTInt. As a note it still had utility
in that it could open intermediate trees, produced from the GRAnalyzer subsystem, as sources, but as this was not done generically
> Since the target libGRAnalyzer does not produce a file named libGRAnalysis, it should be listed as a .PHONY target.
Added as a .PHONY target.
> In the _load_default_style(), we would like style.SetOptStat() to remain uncommented. These are going to be moved to the .grutrc at some point anyways to be configurable. Similarly, we would like for the SetPadTick commands not to be present.
These were not meant to make it in to the pull request and have been removed.
@csullivan
Copy link
Author

csullivan commented Aug 22, 2016

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.

Chris Sullivan added 3 commits August 22, 2016 16:55
> Since the target libGRAnalyzer does not produce a file named libGRAnalysis, it should be listed as a .PHONY target.
In retrospect, libGRAnalyzer is a meta-rule that is marked as a dependency so that make will compile the libGRAnalyzer
submodule. Listing it as a PHONY target thus causes make to recompile all of GRUTinizer each time.
which resulted in those vectors not being copied in when loading from
a rootfile.
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

Successfully merging this pull request may close these issues.

2 participants