search

cms-gem-daq-project / gem-plotting-tools

Repository for GEM commissioning plotting tools
GNU General Public License v3.0
1 stars 26 forks source link

Fix Latency Script #291

Closed dteague closed 4 years ago

dteague commented 4 years ago

Johny noticed an error involving inFile not being defined and hopefully this will fix the script

Description

Moved gemType definition lower in code so inFile is defined before being used

How Has This Been Tested?

Still needs to be tested on gemuser machine where all variables are set/accessible

dteague commented 4 years ago

Testing again these changes with Johny, it still doesn't work. Next commit will be a working version though. Sorry for trying to PR a not working script!

dteague commented 4 years ago

The old was blatantly wrong, referencing a tree that didn't exist in the file at all. Testing on a sample input latency output file outputs pngs as expected.

The problem becomes though how to properly resolve the gemType from the input latency file. In the current iteration, there is no metadata that is in the latency files about gemType. Either we'll have to add that or add a commandline argument to change this

lpetre-ulb commented 4 years ago

The latest solution actually reverts the GE2/1 generalization. Isn't there any other tree in the input latency file which could store the metadata information about gemType?

Also, the PR should probably target release/legacy-1.5 rather than the development branch (or, at least, a back-port should be provided).

dteague commented 4 years ago

With my understanding of what sort of inputs are provided to anaXDAQLatency.py, there are no trees in the input files, only TH1's and none of them have an obvious gemType variable. The input I'm looking at is one provided to me by Johny (/data/bigdisk/GEM-Data-Taking/GE11_QC8/Cosmics/run000456/run000456_allLumi_index000000.analyzed.root)

As for the other branch, once we have a solution to what we should do with the gemType variable, I can open a PR for that branch

mexanick commented 4 years ago

I looked into details, indeed the change for ge21 generalization was introduced there by mistake. I'm not sure we actually want to develop it like that as at some point the plan was changed and we simply use the lightDQM to do all the job while this is just a nice printing around it. However this probably should be shifted towards onlineDQM realm. Finally, there will be a major change in data format at some point in future (calibration data format) which may also touch the latency measurements (not sure about it actually). So far reverting the ge2/1 generalization is fine to me, although if you fancy, a ge2/1 compatibility can be added by passing more (optional, defaulted to GE1/1) parameters.

dteague commented 4 years ago

I talked with @cgalloni and she was of the mind that a command line argument should just be added. Considering this script is standalone, I think this would be the best answer to this problem. Commit incoming

mexanick commented 4 years ago

also please retarget as per @lpetre-ulb suggestion