star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Additon of StFwdTrack to StEvent, updates to StFwdTrackMaker to accomodate #492

Closed jdbrice closed 1 year ago

jdbrice commented 1 year ago

This PR replaces #397 (it was easier to start fresh) Major updates

jdbrice commented 1 year ago

This PR incorporates the discussion that we had over several S&C meetings about the StFwdTracks. Specifically it:

jdbrice commented 1 year ago

commit 0d35ff2 uses the cached STAR geometry in a ROOT file instead of loading a local file. This should resolve the blocking issue on PR #410

plexoos commented 1 year ago

Ok, I added the test from #410 in this PR. Hopefully, it will pass. I am going to close #410 and #397 if you are saying that this PR supersedes the latter and has everything to pass the test.

jdbrice commented 1 year ago

Hi All,

just a reminder to be looking at these changes. We discussed a timeline of about 4 weeks (1 weeks now passed) @starsdong .

Also, @plexoos commits 537b5f6 and 627b4e1 are needed to get the code to compile on SDCC - somehow the include paths are treated differently in the container vs SDCC and this leads to very confusing errors - about the StSPtrVec classes not existing. This is generally fixed by prefixing StEvent includes with the "StEvent/" directory

It would be good to figure out the difference / disconnect and fix it

plexoos commented 1 year ago

Hi Daniel, I see you submit many scripts and macros. Are there any other tests you would suggest to run to verify the performance? We could use the same file st_physics_23072022_raw_4000001.daq or the output produced by the FwdTrack or any other file.

starsdong commented 1 year ago

Dear All, Daniel made quite great efforts in addressing the comments and suggestions. Would it be possible for reviewers to take a look and see whether we can proceed with this PR? Thanks

jdbrice commented 1 year ago

@starsdong @plexoos Any comments that need to be addressed before we resolve this PR?

starsdong commented 1 year ago

I went through the updates from Daniel and I think all the comments are addressed and we are ready to merge. We wait until COB of Monday, in case anyone wants to bring up anything. If no further comment, Dmitri, let us merge this PR. Thanks

plexoos commented 1 year ago

I just fixed a conflict affecting a test using the FwdTrack option. Hopefully, it passes and we can merge 🤞

plexoos commented 1 year ago

The code does not compile due to the following error:

#10 1073.1 g++ -m64 -fPIC -pipe -Wall -Woverloaded-virtual -std=c++0x -Wno-long-long -pthread -Wno-deprecated-declarations -Werror -O2 -g -falign-loops -falign-jumps -falign-functions -Dsl79_gcc485 -D__ROOT__ -DNEW_DAQ_READER -I. -IStRoot -I.sl79_gcc485/include -I/opt/software/linux-scientific7-x86_64/gcc-4.8.5/root-5.34.38-l3v6vso6qgojm4l2ctwjojs6trbt4hpn/include -c .sl79_gcc485/OBJ/StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.cxx -o .sl79_gcc485/OBJ/StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.o
#10 1073.8 .sl79_gcc485/OBJ/StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.cxx: In destructor 'virtual StFcsTrackMatchMaker::~StFcsTrackMatchMaker()':
#10 1073.8 .sl79_gcc485/OBJ/StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.cxx:62:12: error: deleting object of polymorphic class type 'StEpdGeom' which has non-virtual destructor might cause undefined behaviour [-Werror=delete-non-virtual-dtor]
#10 1073.8      delete mEpdgeo;
#10 1073.8             ^
#10 1073.9 cc1plus: all warnings being treated as errors
#10 1073.9 cons: *** [.sl79_gcc485/OBJ/StRoot/StFcsTrackMatchMaker/StFcsTrackMatchMaker.o] Error 1

A quick look reveals that this class https://github.com/star-bnl/star-sw/blob/main/StRoot/StEpdUtil/StEpdGeom.h includes the ClassDef macro and thus defines virtual methods. However, it does not define a virtual destructor which is required in this case. One way to fix this is to make StEpdGeom inherit from TObect