-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to Track Length in the water & Energy Reconstruction #248
Conversation
Hi! I noticed on lines 275 and 276 for FindTrackLengthInWater.cpp, that the MC True track lengths are being multiplied by 100 to convert them to cm. I was under the impression that they were already in cm from MCRecoEventLoader. Looking into that tool, apparently one of them gets converted to cm... I'm going to raise an issue, but I recommend double checking the units for the TrueTrackLengths in MRD and Water |
Hello! So these lines where left by mistake like this from previous versions of the code. I checked the values of the TrueTrackLengthInWater and TrueTrackLengthInMrd I get from the FindTrackLengthInWater tool. It seems that the TrueTrackLengthInWater is in cm and the TrueTrackLenghtInMrd is not. If I don't multiply TrueTrackLengthInMrd by 100 it should be in cm. To be honest I did not give too much attention to the units of the MC track length in the mrd because I'm using the reconstructed one for the analysis. As for the MCRecoEventLoader, I think TrueTrackLengthInWater is saved in m and TrueTrackLengthInMrd in cm. I think we should decide if we want to make any changes to the MCRecoEventLoader tool before I change anything in the FindTrackLengthInWater tool. |
Gotcha, thank you for looking into this and responding! I agree with you that we as a collaboration should decide on any changes to MCRecoEventLoader before FindTrackLengthInWater. I guess I'm confused as to why units of distance/position for the tank are often kept in meters while it's cm for the MRD. I know it's not your doing! Just thought it'd be helpful to get a conversation started on it. Thanks again! |
d87dc70
to
ab60c1d
Compare
I'm a little lost here, you intially say
but then later say
We certainly should be using consistent units throughout. I believe the official annie units of length is meters so if there are instances where things are being saved in cm they should be updated accordingly. Would someone be able to look into what needs changing and report on it in the next software meeting? |
Sorry if I was unclear. So initially I was talking about the values from the FindTrackLengthInWater tool and then about the values from the MCRecoEventLoader tool. From what I can tell the MCParticleProperties tool saves both the TrueTrackLenghtInWater and TrueTrackLengthInMrd in m. But what happens, as James has mentioned, is that in this line only the mrd track length is being converted into cm before being passed into the RecoEvent store. And in the FindTrackLengthInWater tool I'm getting these values from the RecoEvent store and assuming that they were both in meters I'm multiplying them both by 100 to convert to cm so the units for the MC track length in the mrd turn out to be wrong. But yeah we can discuss what needs to be changed in the software meeting! |
Hi Karagiannis, Would you and/or perhaps @jminock be able to make the corresponding change to the MCRecoEventLoader to leave the MRD track in meters, and then do a search through and also make any required changes to other Tools that retrieve this value and expect it to be in centimeters? |
…and converted reconstructed track length in the water and energy in [m] and [GeV] before passing them to the ANNIEEvent store.
Hello Marcus! I fixed the units of the MC track length in the mrd in the FindTrackLengthInWater tool to be in cm for now and we can look into it further when we change the units in the MCRecoEventLoader. I also converted the DNNRecoLength in m and the BDTMuonEnergy in GeV before saving them in the ANNIEEvent store in order to be in standard ANNIEEvent units. |
Sorry, I'm afraid the new Tool here, PlotsTrackLengthAndEnergy, has 15 |
it didn't look like anything needed to be on the heap so i replaced all the heap allocated variables with stack ones, which should fix the leaks. |
Hello Marcus. Sorry, for taking this long to respond. I was getting an error in lines 96-97 and 108-109 of the |
awesome, thanks for checking it and picking that up! |
Updating the related tools to use Boost Stores and the DataModel for transferring data.