sphenix-collaboration/coresoftware

Our big core software repository

coresoftware's Issues

Tracking momentum shift of 2%

I found this while constructing a tool to compare tracking branches prior to approving merges. This is mostly an advertisement of the issue in case someone else runs into it. I'll set about solving the problem (which I think will be a simple recalibration update) before the meeting next week.

Here is sample of the current SVTX performance in central events from the SvtxSimPerformanceCheckReco tool. Other than the momentum shift issue, it seems we are still doing quite well on the purities and finding efficiencies (so good news for us).

EDIT: This could be related to changes in the mag field (this test used the 2d file).

nodenames cannot contain decimal points

It's not explicitly mentioned and I never really thought about it but decimal points in node names interfere with the DST readback. We store the node names as branch names together with their parent PHCompositeNodes which are separated by decimal points. Compared to the slashes used in PHENIX this enables us to call methods on those nodes from the CINT cmd line. If a node name contains a decimal point it interferes with the restoring of the node tree making an identical restore impossible. Original Node Tree during sim:
CLUSTER (PHCompositeNode)/
AntiKt_Cluster_r0.2 (PHIODataNode)
AntiKt_Cluster_r0.3 (PHIODataNode)
AntiKt_Cluster_r0.4 (PHIODataNode)
AntiKt_Cluster_r0.5 (PHIODataNode)

Reconstructed nodes from DST:
CLUSTER (PHCompositeNode)/
AntiKt_Cluster_r0 (PHCompositeNode)/
2 (PHIODataNode)
3 (PHIODataNode)
4 (PHIODataNode)
5 (PHIODataNode)

I put a check for this on my todo list (bail out if someone tries to create a node with a name containing a decimal point)

cemc prototype generates different hits when compiling without optimizer

During the testing of the modified hcal I came across an issue for the cemc prototype. For debugging purposes I had compile g4detectors without optimization which doesn't affect the results for the hcals. But the cemc results are not identical. My assumption is that it is the reading of the xml calibration file which has this hokey double printouts. Alternatively a mix of floats and doubles can cause this behavior. In any case this behavior points to an issue and at least makes debugging code a lot harder

Boilerplate code in shower interface

Can someone explain me the purpose of this interface exposing the private containers?

https://github.com/sPHENIX-Collaboration/coresoftware/blob/master/simulation/g4simulation/g4main/PHG4Showerv1.h#L71-L80

Wouldn't it be easier to just declare these members public?

crash when writing new SvtxTrack to DST

When writing DST output which includes the new SvtxTrack class in simulation/g4simulation/g4hough, I get a crash after a warning

"Warning in TClass::TClass: no dictionary for class SvtxTrack::State is available"

Mike is aware of the issue and will work on it soon.

secondary PHG4Particle removed, but produced PHG4Hit with no energy is not

So this is a funny one I ran into with the new calorimeter evaluator. I see many g4hits with zero energy in the calorimeter (I thought those were pruned). But then I come to a secondary truth particle that I suspect only left zero energy g4hits, it is a rare case anyway, and the g4particle which was added to the map before is found to be missing by eval time.

The end result is a truth hit that doesn't trace anywhere. Maybe someone turned off or disrupted the zero energy hit pruning but left the track pruning on. I can of course easily ignore g4hits with no energy deposit---that would work---but this must be a space issue for the pro.beta tag.

Can someone confirm that they see zero energy g4hits remaining in the output?

Inheritance of PHG4ProjCrystalCalorimeterDetector

cppcheck showed a lot of shadowed variables. I assume it started as a plain copy of the PHG4CrystalCalorimeterDetector base class. This is just the wrong way of going about inheriting and this needs to be fixed before this propagates into other code. To run cppcheck:

cppcheck --enable=all PHG4ProjCrystalCalorimeterDetector.cc

(this also prints out style warnings which are irrelevant). Another issue is the wrong order of includes. One ALWAYS has to put the includes which use quotes (#include "...") first. Otherwise the intended search path (using local inludes first) when using separate include install directories does not work! In implementations one often gets away with it but in header files this is fatal and is extremely hard to debug.

Bad Tracking Performance

Somehow a bug got into the tracking in the past 10 days or so. Here is a comparison of the current performance versus what I had on 04/28 that I made over the weekend:

This was first reported to me by Tony on Friday, and others are noticing so I thought I'd start a ticket so others can follow the process of getting this fixed. I'm hoping around in the git history to find the culprit commit. I've already ruled out Chris's latest g4hit production changes as the source. Next I will rollback through the G4 updates and try to replicate the performance on 04/28 after we submitted the vertexing changes.

PHG4FullProjSpacalCellReco::set_timing_window_defaults

Looking at a current production file like /sphenix/sim/sim01/production/single_particle/2017-01-13/spacal2d/fieldmap/G4Hits_sPHENIX_e-_eta0_8GeV-0000.root, I see

Error: Can't call PHG4FullProjSpacalCellReco::set_timing_window_defaults(0.0,60.0) in current scope /sphenix/sim/sim01/production/single_particle/2017-01-13/spacal2d/G4_CEmc_Spacal.C:398:

from G4Setup_sPHENIX.C in /sphenix/sim/sim01/production/single_particle/2017-01-13/spacal2d.

I think it's ok when you just modify G4_CEmc_Spacal.C to not call a disapparated method, but that could eventually lead to unforeseen consequences.

PHG4UIsession.cc suppresses overlap warnings

Ultimately it's a bad design in G4 - if one enables checks for overlaps in G4PVPlacement, G4 sends the output (OK or not OK) to stdout instead of errors to stderr. Those messages are essential and we cannot suppress them for any verbosity level. I changed the code that it pipes again everything to stdout independent of verbosity level. I left the rest intact - if we find a solution for this we can switch this back.

PHG4CylinderGeom_MAPS class lacks public constructor

I got this message when running the new maps+itt+tpc macro:
Warning in TBufferFile::WriteObjectAny: since PHG4CylinderGeom_MAPS has no public constructor
which can be called without argument, objects of this class
can not be read with the current library. You will need to
add a default constructor before attempting to read it.

root calls the default ctor without arguments when reading objects back from file. One option to fix this would be to give the existing ctor default arguments.

use of gRandom instead of gsl

simulation/g4simulation/g4hough/PHG4TrackFastSim.C and offline/packages/PHGenFitPkg/Example/testPHGenFit.cc use gRandom instead of random numbers from gsl. We discourage it's use (gRandom has shown some quirks in the past). This code also does not initialize the seed so testing code changes which do not influence the results is basically impossible. There are easy gsl based replacements for uniform/gaussian distributions (look in the particle generators under simulation/g4simulation/g4main), use the PHRandomSeed() function to set the seed.

segfault in jet evaluator for hijing hits files

using the standard Fun4All_G4_sPHENIX.C macro with readhits=true and the input file
/gpfs02/phenix/prod/sPHENIX/preCDR/pro.1-beta.3/sHijing/spacal2d/G4Hits_sPHENIX_sHijing-0-4.4fm_0000.root
It crashes in
JetTruthEval.C:63
with the truthjet being a null pointer.

EMCAL towering

This may be a problem buried somewhere in my environment, but in case there is some subtle error in the default EMCAL towering, I thought I'd report it as a possible issue.

In doing some bounds checking on EMCAL towers, I found 256(phi)124(eta) towers instead of the 25696 I expected. I tracked it back this far:

RawTowerGeomContainer* towergeom = findNode::getClass(topNode,"TOWERGEOM_CEMC");

towergeom->identify();

I get 124 eta bins instead of 96:

RawTowerGeomContainer_Cylinderv1: radius: 95, thickness: 12.7, etabins: 124, phibins: 256eta_bin[0](-1.14917, -1.13591) eta_bin[1](-1.13591, -1.12251) eta_bin[2](-1.12251, -1.10896) eta_bin[3](-1.10896, -1.09526) eta_bin[4](-1.09526, -1.08142) eta_bin[5](-1.08142, -1.06741) ... eta_bin[120](1.09526, 1.10896) eta_bin[121](1.10896, 1.12251) eta_bin[122](1.12251, 1.13591) eta_bin[123](1.13591, 1.14917)

Format of RawTowerContainer id and RawTower id differ

Perhaps someone can tell me which of these two objects is doing the right thing. RawTowerContainer considers the combined tower id to be (ieta << 16 | iphi) while the RawTower does (ieta << 12 | iphi). What is the expected behavior?

infinite loop in calo evaluator for inner hcal in hijing

I started to run over /gpfs02/phenix/prod/sPHENIX/preCDR/pro.1-beta.3/sHijing/spacal2d/G4Hits_sPHENIX_sHijing-0-4.4fm_0200.root using the standard Fun4All_G4_sPHENIX.C macro with readhits=true and the jeteval disabled. It runs now for 10 hours, attaching with the debugger indicates it is still at the first event. It seems to be in an infinite loop in CaloTruthEval.C line 171:
for (PHG4HitContainer::ConstIterator g4iter = _g4hits->getHits().first;
g4iter != _g4hits->getHits().second;
++g4iter) {

PHG4Hit* g4hit = g4iter->second;
PHG4Particle* candidate = get_primary_particle(g4hit);
if (candidate->get_track_id() != primary->get_track_id()) continue;
truth_hits.insert(g4hit);

}

the code doesn't seem to find a match for the candidate and keeps "continuing"

Vertex constructed with NAN in position

As reported in #148, vertex with NAN was submitted to loaded to the global vertex map, which crashed jet finder. A fix was implemented in #148 , vertex with NAN in z is rejected from filling into the global vertex map. Nevertheless, it does not solve the problem of how a NAN-vertex get produced in the first place.

From the last simulation meeting, we decided to raise this issue again to keep track of this TODO item after ALD charge response.

RawClusters and RawTowers missing single unique identifiers

The jet clustering algorithms need unique identifiers to keep track of what is in the jet. RawClusters are missing proper unique identifiers (and RawTowers have two ids instead of one). We have branch with new calorimeter objects, so we wait for those to merge to solve this problem or should we add unique identifiers to these possibly-soon-to-be legacy objects? Will that branch also bring in a new CaloCluster Object?

unintended integer division in RawClusterPositionCorrection.cc?

coverity found a possible problem in this file. It does an integer division but it looks like it should be floating point, details are here:
https://www.phenix.bnl.gov/WWW/p/draft/phnxbld/sphenix/coverity/report/coresoftware/offline/packages/CaloReco/

Inner HCAL towers

I didn't debug it the whole way, but this message at the beginning of a run with the default macro:

HCALCELLRECO: total number of slats 256 not multiple of readout combined slats 5 dropping the last 1 slats from reconstruction

from PHG4HcalCellReco seems to suggest that it is still making towers of 5 tiles from the now 256 Inner HCAL gaps instead of 4 tiles/tower. I couldn't quite trace nslatscombined all the way by reading the code but it looks like it may not get set to 4 for the inner.

Remove of non-unique and non-active branches?

@mccumbermike , there is a few branches left in the repository, which do no carry unique commit after their pull requests.

Shall we remove these branches? I think usually GitHub would prompt us to do so after a pull request is approved to merge these branches into the master.

These branches include:

• JETRECO
• PHPythia8
• evaldevel
• tower/cluseter-ids

I hope this help improve clarity to users who are looking for the official branch and the branches that carries the soon-to-come new features.

Remove TPC development branch after merging?

Thanks @mccumbermike and @alandion for merging the TPC branch into master in pull request #35 . Now TPC branch do not carry any new commits. Now I am writing to start a discussion on relocating the TPC development branch:

Shall we remove it from the sPHENIX-Collaboration/coresoftware repository? The goal is to keep the branch structure in sPHENIX main repositories clean, and only carry master, production and few to-be-merged evaluation branches for larger circle of collaborator to test. Currently there is a to-be-merged evaluation branch CaloTower from @nfeege , which we should merge after the pre-CDR simulation runs.

To keep the TPC development on GitHub, we have a few options:

1. For one developer to contribute commits and for TPC experts to use: we can make a fork repository in personal area, add TPC development branch on it, and update/publish it for other TPC experts to use.
   Example is my SPACAL development branch:
   https://github.com/blackcathj/coresoftware/tree/SPACALTower-NewEval-Tower
   which was merged into sPHENIX master after the development and test was finished through pull request #29
   PS: multiple developers can also exchange contributions through pull request between forks.
2. For shared development among multiple developers.
   An easy way to do so on GitHub would be making an organization of tracking group experts, fork coresoftware in that organization, so all tracking expert can commit. And when the software is ready for wider use, we can merge it into sPHENIX through a pull request. Much like personal fork too.
3. If there is good reason to keep TPC branch on sPHENIX repository, I would like to learn too. But in the end, we would like to avoid many test/dev branches on sPHENIX repository, which would become confusing to non-expert to follow their purpose and status.

index bug in simulation/g4simulation/g4hough/SvtxVertex_v1.C

I ran insure over a single particle simulation and it seems to have discovered an out of bounds problem in this code. In the ctor it initializes by

for (int j = 0; j < 3; ++j) {
for (int i = j; i < 3; ++i) {
set_error(i,j,NAN);
}
}

set_error sets variables in _err which is _err[3]
void SvtxVertex_v1::set_error(unsigned int i, unsigned int j, float value) {
_err[covar_index(i,j)] = value;
return;
}

The index is calculated in covar_index:
unsigned int SvtxVertex_v1::covar_index(unsigned int i, unsigned int j) const {
if (i>j) std::swap(i,j);
return i+1+(j+1)*(j)/2-1;
}
If I take the pair of parameters where insure complains (i=0, j=2) covar_index returns 3 which is off by one:

include

using namespace std;

int main()
{
int i = 0;
int j = 2;
if (i>j) std::swap(i,j);
int k = i+1+(j+1)*(j)/2-1;
cout << "k: " << k << endl;
return 0;
}

PHG4TpcSteppingAction will misbehave if UseG4StepsFlag=0

When integrating over multiple G4 steps the current implementation will likely not store any hits or just the info for the first step. I haven't gone through in great detail what will happen but the current implementation will not work

a clone repository in case-insensitive filesystem does not work as expected...

Cloning in case-insensitive filesystem (e.g default MacOS filesystem) case-sensitive path groups collide and only one from the same colliding group is in the working tree while the second appears always as no staged changes....

case-sensitive groups:
'offline/packages/mvtx/MVTXClusterizer.cc'
'offline/packages/mvtx/MvtxClusterizer.cc'
'offline/packages/mvtx/MVTXClusterizer.h'
'offline/packages/mvtx/MvtxClusterizer.h'
'offline/packages/mvtx/MVTXClusterizerLinkDef.h'
'offline/packages/mvtx/MvtxClusterizerLinkDef.h'
'simulation/g4simulation/g4mvtx/PHG4MVTXDigitizer.cc'
'simulation/g4simulation/g4mvtx/PHG4MvtxDigitizer.cc'
'simulation/g4simulation/g4mvtx/PHG4MVTXDigitizer.h'
'simulation/g4simulation/g4mvtx/PHG4MvtxDigitizer.h'
'simulation/g4simulation/g4mvtx/PHG4MVTXDigitizerLinkDef.h'
'simulation/g4simulation/g4mvtx/PHG4MvtxDigitizerLinkDef.h'

Outdated code in PHG4CylinderGeom_Spacalv3?

This file coresoftware/simulation/g4simulation/g4detectors/PHG4CylinderGeom_Spacalv3_2015.cc has 18k LOC and takes a significant time to compile. For those of us who prefer to work with local builds and make sure all dependencies are rebuilt, it is noticeable.

If it is not used in any active studies now (2015 right?), it should be removed. Of course, everyone interested in this file can find it in the git history and restore if needed.

(By looking at the content of the file it appears this should have been a data file with a minimal interface to initialize the data structs)

PHRandomSeed to use rc RANDOMSEED flag?

Let me just put this out here. We have a lot of duplicated code where you only get a PHRandomSeed if the recoconst flag RANDOMSEED is not set. Should we integrate this functionality into PHRandomSeed? That would require either moving this to fun4all (and code changes to include this file from #include <phool/PHRandomSeed.h> to <fun4all/PHRandomSeed.h> or moving the recoconst code to phool with changing that include path in our code. Our code base is not so large that this is a catastrophe. I would prefer to move this to phool since those flags are not really part of fun4all.

I would also change the seed type to int and return the abs() so G4 is happy with whatever comes back from this.