sphenix-collaboration / coresoftware Goto Github PK
View Code? Open in Web Editor NEWOur big core software repository
Home Page: https://wiki.bnl.gov/sPHENIX/index.php/Main_Page
Our big core software repository
Home Page: https://wiki.bnl.gov/sPHENIX/index.php/Main_Page
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).
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)
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
Can someone explain me the purpose of this interface exposing the private containers?
Wouldn't it be easier to just declare these members public?
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.
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?
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.
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.
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.
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.
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.
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.
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.
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)
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?
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"
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.
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?
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/
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.
@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:
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.
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:
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:
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;
}
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
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'
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)
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.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.