mu2e/offline

Offline software for the Mu2e experiment

Offline

Offline software for the Mu2e experiment at Fermilab.

offline's Issues

Remove temporary code to allow backwards compatibility with older files of CRV event data

There is currently one temporary fix in the CRV code to allow the head of Offline/master to work with files containing older CRV event data. There will soon be a new one. Once the older files are declared obsolete, these fixes should be removed.

The first one deals with special treatment of older StepPointMCs that are missing a data member.

The second one deals with the changes to the counter numbering caused by removal of the counters around the cryo window.

legacy scripts

We have a number of legacy scripts, in particular in Mu2eG4, that are no longer runnable. They all share a common 'boilerplate' that has since been consolidated in JobConfig. It's also not clear why scripts in Mu2eG4 are running step and digi simulation and even reconstruction; isn't testing that functionality part of Validation? I recommend purging redundant and obsolete scripts, and refactoring any that remain to use the JobConfig scripts (as Validation does) to reduce maintenance burdent.

CalPatRec helix chisquared has kludge factors

The Chisquared for the helix found by CalHelixFinder includes a kludge factor which biases the comparison to the chisquared of helices found by TrkPatRec. The kludge factor should be removed and the algorithm parameters adjusted accordingly to provide more accurate selection of the best helix.

Prep work for art v3_09_00 - update product fetch idioms

We currently use some product fetch idioms that will be deprecated in art v3_09_00. For a list of idioms, see page 12 of

https://mu2e-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=37842

Grepping on all of the .cc and .hh files in Offline gives:

- getByToken - 8 lines
- getPointerByLabel - 9 lines
- getManyByType - 69 lines
- getMany (excluding getManyByType) - 14 lines

Some, but not all of these have replacements that will work in art v3_08_00. After we upgrade to art v3_08_00, upgrade as many as possible in order to minimize the work when we upgrade to art v3_09_00.

Target Date: before the end of April 2021

Phasing out StepPointMC

Copying a comment from a discussion here
#379 (comment)

I think StepPointMC is a bit of a mess because it is trying to cover too many use cases.
It tries to be both a "point", which ideally should be just a record of particle state at that point (position, momentum,
time, proper time, and a ptr to its SimParticle) and nothing else. Such "points" can be naturally produced
by our infinitesimally thin virtual detectors, and are useful for analysis.
But then there is also the "step", which has things like various kinds of energy deposition, step length, etc.
The "step" information is used by digitization, but different detectors need different infos. For example
visibleEnergyDeposit is of no interest to the tracker. It would be better if each detector produced
their own "steps" without going through StepPointMC, like ExtMon does (see MCDataProducts/inc/ExtMonFNALSimHit.hh )
Then we could have a lean and clean "analysis" object, and a set of detector-defined steps,
without a shared class that has to satisfy everyone.

This thing needs to be changed...

Explanation, etc...

Resolve TrackerConditions<->ProditionsServe dependency loop

Both TrackerConditions and ProditionsService need to use fcl config header files, such as:
TrackerConditions/inc/StrawResponseConfig.hh
TrackerConditions rightly depends on ProditionsService, but the reverse
should not occur, and this config file is the only way it does.
The object is acting like a utility container. This could be resolved two ways:

1. move config headers to ProditionsService
2. move them to a lower library, either 2a) Mu2eInterfaces (already used
   for the Proditions handle header) or 2b) a new directory, ProditionsConfig, for example.

Rob and Dave, let me know what you think

Moving services files out of test directories

To finish the removal of test directory references from validation and production, we will move
Mu2eG4/test/conditions_01.txt -> ConditionService/data
Mu2eG4/test/globalConstants_01.txt -> GlobalConstantsService/data
and update fcl/standardServices.fcl

The following fcl files reference these files directly, when they would ideally reference them indirectly through prologs in fcl/standardServices.fcl. Here are three options, increasing in difficulty:

1. let them become broken
2. edit the file paths, but don't change to prologs
3. change to prologs

The last requires looking at each file to see if it is doing something non-standard. Please comment. Also please comment if any of these are in current use, and should be kept running as a high priority.

Analyses/fcl/PionMomentumAnalyzer.fcl
Analyses/fcl/pbars1hist.fcl
Analyses/fcl/simParticleAnalyzer.fcl
Analyses/test/ConversionEnergyLoss.fcl
Analyses/test/InteractiveRoot.fcl
Analyses/test/InteractiveRoot2.fcl
Analyses/test/SelectiveStepPtPrinter_g4s1.fcl
Analyses/test/TVirtDebug.fcl
Analyses/test/TestTO.fcl
Analyses/test/cosmicFilter.fcl
Analyses/test/extmon_uci_readback.fcl
Analyses/test/genMixReco.fcl
Analyses/test/histforpabs.fcl
Analyses/test/hitDisplay.fcl
Analyses/test/isoRPCtest.fcl
Analyses/test/materailsPropStudy.fcl
Analyses/test/mixPointsDump.fcl
Analyses/test/printStrawHits.fcl
Analyses/test/printTrackerGeom.fcl
Analyses/test/psVacuum_readback.fcl
Analyses/test/readCaloDigi.fcl
Analyses/test/readMCTrajectories.fcl
Analyses/test/readStoppedPis.fcl
Analyses/test/readTrackCluster.fcl
Analyses/test/readback0.fcl
Analyses/test/simParticleCheck00.fcl
Analyses/test/simParticlesWithHitsExample.fcl
Analyses/test/stoppingTarget00.fcl
Analyses/test/transportMuonStudy.fcl
Analyses/test/transportMuonStudy_onlyTransport.fcl
Analyses/test/vd_readStage2.fcl
Analyses/test/vd_readback.fcl
BFieldGeom/test/makeBinaryMaps.fcl
BFieldGeom/test/readBinaryMaps.fcl
BFieldTest/test/BFieldSymmetry.fcl
BFieldTest/test/BFieldTest.fcl
CaloCluster/test/CaloClusterProduction.fcl
CaloCluster/test/ClusterTrajectory.fcl
CaloCluster/test/readCaloCluster.fcl
CaloCluster/test/readCaloClusterEff.fcl
CaloCluster/test/readCaloClusterEnergyResolMap.fcl
CaloCluster/test/readCogRecFunc.fcl
CaloCluster/test/readCrystalHit.fcl
CaloCluster/test/readOffSetCog.fcl
CalPatRec/test/calPatRec.fcl
CalPatRec/test/calPatRec_01.fcl
CalPatRec/test/calPatRec_02.fcl
CalPatRec/test/calPatRec_display.fcl
CalPatRec/test/calPatRec_display_cnv00502.fcl
CalPatRec/test/calPatRec_display_read.fcl
CalPatRec/test/deltaFinder_debug.fcl
CalPatRec/test/deltaFinder_debug_ce.fcl
CalPatRec/test/deltaFinder_test.fcl
CalPatRec/test/mergePatRec.fcl
CalPatRec/test/mergePatRec_02.fcl
CalPatRec/test/mergePatRec_03.fcl
CalPatRec/test/mergePatRec_display.fcl
CalPatRec/test/mergePatRec_display_read.fcl
CalPatRec/test/printTrackerNumerology.fcl
CalPatRec/test/trkPatRec_01.fcl
CalPatRec/test/trkPatRec_02.fcl
CalPatRec/test/trkPatRec_display.fcl
CalPatRec/test/trkPatRec_display_read.fcl
CRVResponse/efficiencyCheck/CRV_Efficiency_check.fcl
CRVResponse/singleCounter/dicounter_across.fcl
CRVResponse/singleCounter/singleCounter.fcl
CRVResponse/singleCounter/singleCounter_across.fcl
CRVResponse/singleCounter/singleCounter_darkNoise.fcl
CRVResponse/singleCounter/singleCounter_point.fcl
CRVResponse/singleModule/singleModule.fcl
CRVResponse/test/CRVResponseDAQ.fcl
CRVResponse/test/CRVResponsePlot_v05.fcl
CRVResponse/test/CRVResponseReco.fcl
CRVResponse/test/CRVResponse_RadiationBackgroundStudies.fcl
CRVResponse/test/CRVResponse_v05.fcl
CRVResponse/test/CRVResponse_v07.fcl
EventGenerator/fcl/cryTest.fcl
EventGenerator/test/FromStepPointMCs.fcl
EventGenerator/test/ceLeadingLog_test.fcl
EventGenerator/test/ceMEndpoint_test.fcl
EventGenerator/test/ceMLeadingLog_test.fcl
EventGenerator/test/cePEndpoint_test.fcl
EventGenerator/test/cePLeadingLog_test.fcl
EventGenerator/test/g4test_FromStepPointMCs.fcl
EventGenerator/test/corsikaTest.fcl
EventGenerator/test/rpc_int_test.fcl
EventMixing/test/inspectMixed.fcl
EventMixing/test/inspectUnMixed.fcl
EventMixing/test/mixProducer_01.fcl
ExtinctionMonitorFNAL/test/EMFBoxFluxAnalyzer.fcl
ExtinctionMonitorFNAL/test/EMFRoomFluxAnalyzer.fcl
ExtinctionMonitorFNAL/test/digiDefsCommon.fcl
ExtinctionMonitorFNAL/test/digiTuning.fcl
ExtinctionMonitorFNAL/test/emfRecoTrackAnalysis.fcl
ExtinctionMonitorFNAL/test/extMonFNALDefsCommon.fcl
ExtinctionMonitorFNAL/test/g4detectorDefsCommon.fcl
ExtinctionMonitorFNAL/test/g4s1_channelTest.fcl
ExtinctionMonitorFNAL/test/g4s1_emfMARSRoomHistos.fcl
ExtinctionMonitorFNAL/test/recoDefsCommon.fcl
ExtinctionMonitorFNAL/test/trajectoryIn.fcl
fcl/standardServices.fcl
JobConfig/beam/DS_pions.fcl
JobConfig/beam/TGTpionstops.fcl
JobConfig/beam/TS-CRV.fcl
JobConfig/beam/beam_defs_g4s1.fcl
JobConfig/beam/beam_test_4_replicated.fcl
JobConfig/cosmic/filterStage1.fcl
JobConfig/extmon/extmonbeam_g4s2.fcl
JobConfig/pbar/resample_vertex_s1.fcl
JobConfig/pbar/resample_vertex_s2.fcl
JobConfig/pbar/vertex_s0.fcl
JobConfig/pbar/vertex_s1.fcl
JobConfig/pions/pions_g4s1.fcl
JobConfig/pions/pions_g4s2.fcl
JobConfig/pions/pions_g4s3.fcl
JobConfig/pions/pions_g4s4_IntConv.fcl
JobConfig/pions/pions_g4s4_RPC.fcl
JobConfig/pions/pions_nts3tgtstops.fcl
JobConfig/test/MuonTransport/MuonTransportSingleStage.fcl
JobConfig/test/MuonTransport/MuonTransportStage1.fcl
JobConfig/test/MuonTransport/MuonTransportStage2.fcl
JobConfig/validation/mixPointsDump.fcl
JobConfig/validation/psEnclosureBeamWindow.fcl
JobConfig/validation/psEnclosureExtMonWindow.fcl
Mu2eG4/fcl/iontest_g4s2.fcl
Mu2eG4/fcl/rantest.fcl
Mu2eG4/fcl/replayWithSkip.fcl
ParticleID/test/test.fcl
Sources/test/FromEMFMARSFileWeighted.fcl
Sources/test/testFromExtMonFNALMARSFile.fcl
TrackCaloMatching/test/caloMatching.fcl
TrackCaloMatching/test/readCaloMatching.fcl
TrackCaloMatching/test/readExtrapol.fcl
TrackCaloMatching/test/trkExtrapol.fcl
TrkExt/test/TrkExt.fcl
TrkFilters/fcl/ReadDigisCalo.fcl
TrkFilters/fcl/TTDigis.fcl

Test Issue

Test test test @brownd1978 @sophieMu2e

first markdown heading

here is some text

code snippet:
for (int i = 0; i < 10; i++) { } // code

Can reference Pull request/Issues like this #30

second markdown heading 2

• Markdown
• Bullet
• Points

task list

• task 1
• task 2
• task 3
• task 4

Scrub Mu2eUtilities for inappropriate #includes

There are a lot of files in Mu2eUtilities that include art headers for no reason. Remove them.

SensitiveDetectorHelper monolith

The SensitiveDetectorHelper class holds data structures to accumulate detector hits, which must be per-thread. But it also contains code like registerSensitiveDetectors(), which needs to be called once per job. The user of the latter code, Mu2eG4MTRunManager, gets all the data collections as its nested members, and they are not per-thread. This is confusing and unnecessary. It would be better to separate the data storage part from the detector registration.

What determines the timing of label updates on PRs?

There have been 4 merges into Offline/master in the last few days.
#289 Dec 9, 10:55 AM
#291 Dec 9, 11:53 AM
#283 Dec 10, 1:09 PM
#282 Dec 10, 3:09 PM

Now look at the history of PR #284

• the tests completed Dec 2, 11:12 AM
• the tag was swapped to "build pending" on Dec 9, 4:04 PM

First question: what determines the timing when the tag is swapped?

Now look at PR #287

• I ran the tests on Dec 9, 10:28 AM
• Test completed Dec 9, 10:51 AM - sets the "build finished" tag
• I ran the tests on Dec 10, 4:10 PM

Note the tag stayed at "build finished" past all 4 merges mentioned above and never changed to "build pending".

Second question: why was this tag never changed to "build pending" ? Is it the same timing issue as the first question or something different?

MC-free builds for use in the trigger and other online uses

A few years back we set a goal of producing an MC-free build for use in the trigger. We have been slowly moving in this direction as we remove MC-awareness out of algorithms and data products that will be used on data. It will soon be time to assess how close we are to getting there.

A corner case came up the other day. We need an event display in both the online and offline worlds; in the offline world we need it to have powerful MC-aware functionality. An event display is big enough that we must not have two things to maintain. At an appropriate time we should discuss this and make a plan.

Here some thoughts towards seeding that discussion when the time comes:

1. Descope the concept of an MC-free online build to allow limited MC-aware functionality, such as the event display and code needed to support it.
2. Put the MC-aware elements of the event display into a tool that is not built for the online distribution
3. Use #ifdefs to enable an MC-free build of the event display
4. Give up on the idea of an MC-free online build entirely

At a minimum we should produce some data-like data sets as part of MDC2020 and verify that the event displays work on those files.

Calorimeter template waveform is deprecated and about 4 times shorter than the real one.

We need to update the template used to generate the simulated SiPM response in the calorimeter simulation. The current waveform is about 4 times shorter than the one obtained with the latest version of the readout electronics.

Offline JobConfig dependence

JobConfig will be split off to its own repo. Here is a summary of Offline dependence on JobConfig. All these will need to be dealt with in order to do the split cleanly. Or maybe each can be fixed when needed?

********** using JobConfig/common/geom_baseline.txt
Analyses/fcl/PionMomentumAnalyzer.fcl
Analyses/fcl/pbars1hist.fcl
Mu2eG4/fcl/iontest_g4s2.fcl
EventGenerator/fcl/cryTest.fcl
ExtinctionMonitorFNAL/test/emfRecoTrackAnalysis.fcl
Mu2eG4/geom/studies/geom_cosmic_v05.txt

********** non-trivial - move to Production?
using #include "JobConfig/reco/Reco.fcl"
TrkDiag/fcl/TrkAnaDigisReco.fcl

using #include "JobConfig/reco/prolog.fcl"
Mu2eKinKal/fcl/SeedTest.fcl

********** event displays - move to Production?
using #include "JobConfig/reco/mcdigis_CRY.fcl"
EventDisplay/fcl/EventDisplayDigis.fcl

using #include "JobConfig/reco/mcdigis_nocosmic.fcl"
TEveEventDisplay/CallerFclExamples/cosmicExample.fcl
TEveEventDisplay/CallerFclExamples/cosmictracks.fcl

using #include "JobConfig/beam/prolog.fcl"
TEveEventDisplay/src/oldprolog.fcl

********** this will be split off in its own repo,
maybe move this fcl to that repo
using #include "JobConfig/alignment/collect_nofield_cosmictracks.fcl"
#include "JobConfig/reco/misalign_epilog.fcl"
TrackerAlignment/fcl/job_template.fcl

********** in test, expert can fix
Analyses/test/Mu2eCCFC_conversion.fcl
Analyses/test/Mu2eCCFC_dio.fcl
Analyses/test/genMixReco.fcl
Analyses/test/readTrackCluster.fcl
Analyses/test/vd_readStage2.fcl
EventGenerator/test/ceLeadingLog_test.fcl
EventGenerator/test/ceMEndpoint_test.fcl
EventGenerator/test/ceMLeadingLog_test.fcl
EventGenerator/test/cePEndpoint_test.fcl
EventGenerator/test/cePLeadingLog_test.fcl
EventGenerator/test/corsikaTest.fcl
EventGenerator/test/testStoppedParticleReactionGun.fcl
TrkPatRec/test/TrkPatRecTestMix.fcl

********** will move to Production
CampaignConfig/corsika.cfg
CampaignConfig/mdc2020.cfg
Validation/fcl/ceMixDigi.fcl
Validation/fcl/ceSimReco.fcl
Validation/fcl/cosmicSimReco.fcl
Validation/fcl/reco.fcl
Validation/test/ceDigi.fcl
Validation/test/ceMix.fcl
Validation/test/ceSteps.fcl
Validation/test/muSteps.fcl

********** not an include
EventMixing/fcl/prolog.fcl

Kalman filter adaptation of cosmic fit

Kalman filter adaptation of @sophiemiddleton 's cosmic fit

• Using TrkStrawHit and KalFit
• Will be easier after the transition to a kinematic (6-parameter) fit

Opening this issue from the project since it looks like this is now being developed / discussed.

Needed for #61 .

Agree with filename recommendations

I agree with Ray's recommendation that the filenames of output files ( art, root ... ) should not be differentiated mt/non-mt.

Ray, if we remove the MT fcl files, would we then have an additional fcl layer on top of the base non-MT layer if we want to run MT?

I would leave the MT fcl files, but change the process name and output file name in the MT fcl files to be the same and nn-MT, or better, the MT fcl file becomes one to two lines, taking the names from non-MT.

Originally posted by @rlcee in #145 (comment)

Proton response nonlinear when _drift1e is not enabled

in StrawDigisFromStrawGasSteps_module.cc the response with and without _drift1e enabled is different for protons. Without _drift1e there is a much larger variation in the output pulse size than expected.

Resolve BTrkData<->RecoDataProducts dependency loop

Please remove
#include "BTrkData/inc/TrkStrawHit.hh"
from
RecoDataProducts/inc/TrkToCaloExtrapol.hh
to remove dependency loop. BTrkData needs to
depend on RecoDataProducts, but the reverse is not appropriate.

Define explicit trigger_paths for production jobs

Converting PR#136 to an issue
#136

Get rid of magic numbers in Offline/DAQ code

This issue was generated during the review of pull request #158. The issue will be closed without resolving this and it needs to be addressed later.

There is one unresolved issue in that PR that we cannot address until some other work has been done. The code has some hard coded magic numbers. This code is not the authoritative source for these numbers; we need to figure out the authoritative source and the code should get those numbers from that source. There are at least 3 stakeholders involved in defining the authoritative source: some of the numbers will be present in firmware that runs on the DAQ hardware; some of the numbers may be used within mu2e_artdaq to understand the data payload; and the numbers may be used in several places within Offline, including the code reviewed here and the code that produces binary format data from simulated digis. The values of some of the magic numbers may be time dependent.

It's likely that the firmware will never have a history of the values - when they need to be changed, they will be changed in the source, the code rebuilt and distributed. I don't know the plans of the online group - do they plan to track the history of these values or do they only need to know the current value. For Offline we will need the full history, with IOVs, in Productions. We also need to define a process to ensure that when the firmware people make changes, that everything is copied to online and Proditions.

Missing interface to pass to the Calo-clustering modules the list of dead channels

Resolve Mu2eUtilities<->RecoDataProducts dependency loop

Please move
Mu2eUtilities/inc/BuildLinearFitMatrixSums.hh
Mu2eUtilities/inc/TwoLinePCA.hh
to GeneralUtilities to prevent these references
Offline/RecoDataProducts/inc/CosmicTrack.hh:#include "Mu2eUtilities/inc/BuildLinearFitMatrixSums.hh"
Offline/RecoDataProducts/src/StereoHit.cc://#include "Mu2eUtilities/inc/TwoLinePCA.hh"
causing a dependency loop (Mu2eUtilities depends on RecoDataProducts,
so the reverse is not appropriate)

More careful review might result in a different recommended move.

Legacy Scripts

We have a number of legacy scripts, in particular in Mu2eG4, that are no longer runnable. They all share a common 'boilerplate' that has since been consolidated in JobConfig. It's also not clear why scripts in Mu2eG4 are running step and digi simulation and even reconstruction; isn't testing that functionality part of Validation? I recommend purging redundant and obsolete scripts, and refactoring any that remain to use the JobConfig scripts (as Validation does) to reduce maintenance burdent.

Intermittent failures of g4test_03MT

Look at the conversation for PR #263 . In the first build g4test_03MT failed. I made another trivial change and it passed. remember seeing this before but I don't remember details. There are currently some known issues in the shutdown of the G4MT that have random behaviour. Lisa is working on it. I don't know for sure that this is the same issues as the one Lisa is working on.

the calorimeter simulation doesn't include any saturation effect in the channel response

Need to implement channel maps for the sub-detectors

We need to implement a method that assigns the proper channel-Id using as input the "hardware" coordinates given by the ROC and channel Id defined in the ArtFragment.
This method will be used in DAQ/src/StrawAndCaloDigiFromFragment_module.cc (and in the analogous for the CRV, STM and EXTM).

Legacy Scripts

We have a number of legacy scripts, in particular in Mu2eG4, that are no longer runnable. They all share a common 'boilerplate' that has since been consolidated in JobConfig. It's also not clear why scripts in Mu2eG4 are running step and digi simulation and even reconstruction; isn't testing that functionality part of Validation? I recommend purging redundant and obsolete scripts, and refactoring any that remain to use the JobConfig scripts (as Validation does) to reduce maintenance burden.

const correctness in BFieldManager ?

Something to check:

Consider the following fragment from BFieldGeom/inc/BFieldManager.hh:

class BFieldManager {
public:
typedef std::vector<std::shared_ptr> MapContainerType;
const MapContainerType& getInnerMaps() const { return innerMaps_; }
private:
MapContainerType innerMaps_;
};

I think that the const accessor getInnerMaps() actually gives non-const access to the Maps! The const only protects the vector itself, not the BFMap objects to which the entries point.

I am not sure the easiest way to fix this. Can we do something like:
typedef std::vector<std::shared_ptr> MapContainerType;

Does that cause problems creating the BFMap objects? If so we can make a transient object of type std::vector<std::shared_ptr> and then copy the final result to the data member?

Are there similar issues in the proditions system which also makes use of shared_ptr?

Cosmic Track Selector

• Select high momentum based on observable parameters
• Select tracks most useful for alignment and calibration
• Tracker coordinate accessors

Define best practices for using objects from Proditions.

Define best practices on the wiki; scrub the existing code and what for issues during code reviews.

Here some initial thoughts on best practices.

Proditions returns shared_ptr. We should not pass this down the call chain to where it is used. At a minimum we should derefence it to a T const& once at the start of produce and pass that down the call chain. Even better, if T has a lot of structure, extract a const& to the lowest level object that is needed to do the job - then pass that down the call chain. It's OK to plan for future evolution and pass a const& to a higher level object than is needed for today's implementation.

Why? The structure of Productions and Mu2e Offline guarantees that if the shared_ptr is valid at the start of produce it will remain valid throughout produce. Since the deference does safety checks, we only need to pay the price of the safety check once per call to produce. Moreover it is more expensive to pass a shared_ptr as an argument than it is to pass a T const&. If we are playing the call and dereference overhead in a loop or nested loop it will add up.

CaloReco/fcl/prolog_trigger.fcl is overwriting module instances created by prolog.fcl

The calo trigger prolog is using the same module instance names as the regular reco sequence, which means the behavior of Oflfine depends on the order in which they are sourced, creating unexpected and sometimes erroneous results.

Tags did not propagate

Earlier today I updated the fork of Offline in my github account to the head of master. I tagged the head of master as v7_5_5; I merged in Ray's e19 changes and then tagged again as v7_5_6. Then I requested a pull of my fork to Mu2e/Offline/master.

I just checked out Mu2e/Offline and the tags are missing.

Does anyone know what I did wrong and how to fix it? Maybe it does not make sense to expect that tags on a fork will survive a merge?

I can fix it by cloning directly from Mu2e/Offline, tagging and pushing. I think that I have permission to do that.

Mu2eG4 configuration regression

A while ago we moved away from using SimpleConfig to specify g4 simulation runtime behavior, with the intent to keep that technology for just geometry information. I see that constructs like

  if( _config.getBool("PS.Vacuum.Sensitive", false) ) {
    psVacuumLogical_->SetSensitiveDetector(psVacuumSD);
  }

are creeping in again. The above snippet is from the HEAD of Mu2eWorld.cc. It did not exist in e.g.
/cvmfs/mu2e.opensciencegrid.org/Offline/v7_0_0/SLF7/prof/Offline/Mu2eG4/src/Mu2eWorld.cc

Track hit reco and pattern recognition uses unaligned geometry

The code producing ComboHits and performing pattern recognition currently uses the unaligned geometry. Those modules need to be refactored to use the aligned tracker geometry.

Calorimeter has duplicate volumes

setStepLimitToAllSuchVolumes WARNING: found caloBackPlateFEELog 2 times
setStepLimitToAllSuchVolumes WARNING: found caloCrystalFrameInLog 2 times
setStepLimitToAllSuchVolumes WARNING: found caloFEEBoxInLog 2 times
setStepLimitToAllSuchVolumes WARNING: found caloFrontPlateLog 2 times
setStepLimitToAllSuchVolumes WARNING: found caloFullBackPlateLog 2 times
setStepLimitToAllSuchVolumes WARNING: found caloHoleBackLog 2 times
setStepLimitToAllSuchVolumes WARNING: found calofullCrystalDiskLog 2 times

Trigger path identification changes with configuration

Trigger paths are currently identified in the datastream by their index in the art TriggerResult object. That index can change depending on the number and order of paths run. We need to define a 'permanent' TriggerPathID associated with a particular trigger line (say tprSeedDeM) so that validation and analysis selection can keep track of these. I suspect the existing trigger tables can be leveraged to implement this.

Hardcoded physics values in Mu2eUtilities/src/BinnedSpectrum.cc

The code in Mu2eUtilities/src/BinnedSpectrum.cc contains things like

  const double bindingEnergyFit{0.464};
  const double recoilEnergyFit {0.220};
  const double deltaMassFit    {3.121};

which are wrong for non-aluminum targets, or

  double ehi = 105.; // cut off at muon mass

Remove or improve beginRun Proditions access

On 3/24/21 at the comp/soft meeting we agreed that accessing
Proditions in beginRun and beginSubRun should be removed
until it is proven to be needed. The following modules already use it
TrackerMC/src/StrawDigisFromStrawGasSteps_module.cc
TrkHitReco/src/StrawHitReco_module.cc
TrkPatRec/src/KalSeedFit_module.cc
The dangers include caching conditions or derived items when
the underlying conditions are changing, and doing unnecessary database
retrievals when there are no events in the run/subrun.
If the need is proven, we should look for ways of mitigating the
dangers

Management of PhysicalVolumeInfoMultiCollection

In MC production we currently store the PhysicalVolumeInfoMultiCollection. To reduced file size we compress the collection for each subrun to remove uninteresting volumes. Then we store the compressed object in the art SubRun object. Even with this compression it can be a big payload in sparse skim.

Another option is to store the uncompressed object in the art Run object. When we concatenate input files, use the data product aggregate function to check that the payloads are identical. This ensures exactly one copy of the geometry per file. We can choose to do some compression if needed.

This adds a new boundary condition to workflow design. If we change the geometry mid stage in a sim campaign. Then we need to start a new run.

Recover Proditions thread safety

PR #198, modified by PR #202, introduced root TH1F/D class to
TrackerConditions/src/StrawResponseMaker.cc
Proditions was designed to be thread-safe, but this call breaks
that design. This will need to be replaced by a thread-safe utility function.

Synchronizing pdgcodes across Offline, BTrk and KinKal is a bigger issue. Fortunately they all use the same values. It's clear though this code should use PDGCode values, not hardcoded numbers.

Synchronizing pdgcodes across Offline, BTrk and KinKal is a bigger issue. Fortunately they all use the same values. It's clear though this code should use PDGCode values, not hardcoded numbers.

Originally posted by @brownd1978 in #286 (comment)

Mu2eG4 internal filtering

There is ad-hoc code inside Mu2eG4 that is configured with SDConfig.inputs, SDConfig.cutMomentumMin, SDConfig.minTrackerStepPoints. The above parameters were added with the comment: "changes made to move FilterStepPointMomentum and TrackerStepPointFilter functionality into the Mu2eG4 module so that we can reduce the size of the EventStash and not waste as much time in art::commit in MT mode for CRY runs".

We do not use EventStash any more. I would like to remove these config parameters and related code. If they are still needed and important, I would like proponents to quantify the CPU saving achieved by using internal filtering with the current code.

Using them breaks the model of each producer module creating all data products, even if emtpy, in each event. We need to either eliminate the filtering, or make Mu2eG4 a filter module.

Andrei

Custom objects in TTrees can't be read in Python

The Python package uproot is becoming the de facto standard for converting TTrees into Numpy arrays or Pandas dataframes. Unfortunately, it is not able to read custom objects (and I think PyROOT also have this issue).
In particular, I am referring to what happens e.g. in ComboHitDiag (but also other analyzers) where we store positions and directions as XYZVec. In my opinion, we should try to store this type of information as flat objects (for example three floats like pos_x, pos_y and pos_z) or fixed-size arrays (which can be read by uproot) in order to make TTrees as widely accessible as reasonably possible.

Edit: in the past I opened an issue on the uproot github where the author confirmed that there is no easy way to read these objects scikit-hep/uproot3#418.

Unbiased residuals

Implement unbiased residual calculation for alignment and drift calibration

Add total charge to StrawDigiMC

The total charge from all the ionizations for a digi should be added to StrawDigiMC for diagnostics.

Stop using NaN's as a way to return status in CRV reco pulses

At the Wed Aug 19, 2020 weekly software and computing meeting Ralf reported that when a CRV reco pulse fails the fit he used to discard it. In the interest of improving efficiency he now keeps the pulse but sets some values to NaN; other values he can set to the value used as input to the fit (determined by an earlier step).

We request that he add a status word that describes the status of the reco pulse and never set a value to NaN; instead set those values to "safe" values. Presumuably there might be many classifications of pulses so don't use a bool to flag good/bad; instead use something like a bitmap to to hold the status. See, for example, RecoDataProducts/inc/StrawHitFlag.hh .

If a parameter is normally provided by fcl, we do not want default values in the code. Same comment in many places.

If a parameter is normally provided by fcl, we do not want default values in the code. Same comment in many places.

Originally posted by @kutschke in #286 (comment)

Compile and link options

We have not reviewed the compiler and link options for a while. It's time to do it again.

When we do this consider adding -Wshadow to catch identifiers that shadow others. A quick look suggest that this might be a lot of work. As of commit e9b5b8e on March 18, scons -k gives 39,324 lines of error messages of which 548 lines are unique. The vast majority involve third party libraries so it's not clear if this is practical. In any case it's lower priority than a review of options.

use of std::move ?

Some modules use std::move in a way that prevents copy elision (an optimisation where a function return value is created in memory outside function stack memory.)

An example from a recent run of clang-tidy in PR #254 shows examples of this in GeometryService/src/ProductionTargetMaker.cc.

https://gist.github.com/FNALbuild/dc7385aec847b7037c7e7dce62d3e570#file-clang-tidy-log

Although this is in the branch Mu2eII_SM21 this code is also present in master.

FNALbuild now uploads logfiles to GitHub gists, including clang-tidy recommendations, meaning this output should be more accessible to everyone from now on.