ANNIEsoft / ToolAnalysis

Other
10 stars 53 forks source link

Update to Track Length in the water & Energy Reconstruction #248

Closed ckaragian closed 8 months ago

ckaragian commented 1 year ago

Updating the related tools to use Boost Stores and the DataModel for transferring data.

jminock commented 1 year ago

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

ckaragian commented 1 year ago

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.

jminock commented 1 year ago

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!

marc1uk commented 1 year ago

I'm a little lost here, you intially say

It seems that the TrueTrackLengthInWater is in cm and the TrueTrackLenghtInMrd is not.

but then later say

I think TrueTrackLengthInWater is saved in m and TrueTrackLengthInMrd in cm.

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?

ckaragian commented 1 year ago

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!

marc1uk commented 10 months ago

Hi Karagiannis, really sorry this has been left hanging for so long. Can you remind me what's going on with this? From your last message it sounds to me like the RecoEvent store is saving MRD track lengths in cm when, to be consistent with other standard ANNIEEvent units, it should be left in meters. If your FindTrackLengthInWater Tool needs things in centimeters, it can do the conversion internally, but again I would recommend ensuring any relevant lengths saved back to the DataModel are in meters.

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?

ckaragian commented 10 months ago

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.

marc1uk commented 10 months ago

Sorry, I'm afraid the new Tool here, PlotsTrackLengthAndEnergy, has 15 new calls and no delete calls. I realize it's probably not going to matter too much as it only runs once on Initialise, but can I ask you to clean up the memory leaks? As much as anything else novice coders tend to use others' code as a point of reference, so it's not good to have this being in the codebase as it gives a bad example.

marc1uk commented 10 months ago

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. I would appreciate if you could test this though, as i just did it on the github interface so can't try it out. If for any reason this isn't suitable feel free to revert the commit if you need to and implement your own.

ckaragian commented 8 months ago

Hello Marcus. Sorry, for taking this long to respond. I was getting an error in lines 96-97 and 108-109 of the PlotsTrackLengthAndEnergy tool. It seems that the AddEntry() method of the TLegend object gets pointers as arguments so I was getting errors when compiling but I fixed that. I also tested it out to check if anything else is wrong but everything seems to be working as it is supposed to. Please let me know if I need to look into something else!

marc1uk commented 8 months ago

awesome, thanks for checking it and picking that up!