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

Ge21 generalizing #252

Closed dteague closed 5 years ago

dteague commented 5 years ago

PR for changing the gem plotting code to be compatible with GE21 plotting

Description

The gem-plotting code is hardcoded with many magic numbers that are specific to the GE11, so with changes in the tree structure to add gemtype and detectortype, the plotting code can be expanded to allow for GE21 plotting that is determined on the fly by this gemType variable.

The bulk of the code is removing instances of 24, 8, 3, and combinations of these to more generic variables, ie nVFATS, maxiEta, maxiPhi. These variables are derived from chamber_maxiEtaiPhiPair in ./mapping/chamberInfo.py and from vfatsPerGemVariant in gempython.tools.hw_constants All of the mapping/chamberInfo.py variables have been turned into dictionaries that require the gemType to get the correct variables, ie

chamber_vfatPos2iEta => chamber_vfatPos2iEta["ge11"] etc.

Since a large code change is the actual canvas creation code, the largest change is to canvas making. The makeAxBCanvas has been removed in place of merging this functionality into the saveSummaryCanvas to make a new function called getSummaryCanvas. This means the function now always returns a canvas created and there is a flag to decide to save the canvas or not. ie function def is now

canv = make3x8Canvas(...)    =>    canv = getSummaryCanvas(..., gemType="ge11")
saveSummaryCanvas(...)    =>    getSummaryCanvas(..., gemType="ge11", write2Disk=True)

Also, because the makeAxBCanvas function also added histograms to an existing canvas, this functionality has be put to a helper function addPlotToCanvas which hopefully will make the code more readable as well. So the change becomes

canv = make3x8Canvas(initialContent=hist1, ...)
canv = make3x8Canvas(initialContent=hist2, canv=canv, ...)

=>

canv = getSummaryCanvas(hist1, ..., gemType="ge11")
canv = addPlotToCanvas(canv, hist2, gemType="ge11")

Last, many functions include now a gemType variable that needs to be supplied to give the right gemType. The default is "ge11", so the code remains backwards compatible for all cases where the gemType isn't actually supplied.

Types of changes

Motivation and Context

This change has mainly be motivated to be backwards compatible and to default to ge11 in all unknown cases. Since ge21 is needing plots specifically made for it (and not ones derived from ge11 algorithms), having code that can flexibly handle ge21 and in the future, me0 is a must.

For further information on this, look at issue #226

How Has This Been Tested?

For testing, the code was run over ge11 and ge21 files with the internal gemType flag flipped (ge21 files didn't have that tree in it).

NOTE: currently, only the main run files (ie files in the top directory) are changed. Files in the ./macros area have not been touched. Here is a list of what has been done.

Checklist:

jsturdy commented 5 years ago

when you go to address comments, you should rebase onto the latest cms-gem-daq-project/develop

dteague commented 5 years ago

I do have some questions as shown by the comments to code blocks, and after those are answered, I can fix and test on files again to make sure everything is working correctly

jsturdy commented 5 years ago

OK, it would probably be good to sit together and go over the rebase workflow, as your previous attempt succeeded (but in a sub-optimal way, as you created a new merge and now have a bunch of duplicate commits)

dteague commented 5 years ago

I looked into this and I removed the extraneous commits and rebased them on the newest changes. The commit log shows only the 6 commits on top of the develop branch, so all seems to have worked, but this might need to be looked over since my git skills are still pretty poor :stuck_out_tongue_closed_eyes:

dteague commented 5 years ago

Ran tests and made minor changes. Tests are the same as before, namely:

I did notice that anaSBitMonitor gave strange results for GE11, mainly that all vfats pointed to vfat 7 and were misID'd the same number of times for each vfat. Considering not much was changed with this file, I'm inclined to believe this is a problem with the file I'm running over, not the changes made

All of the gemType reading in is surrounded by comments saying ##### FIXME so when a proper gemType read in is decided, that can be changed quickly. For all of these tests, I just changed the default value to "ge21" and recompiled.

If anyone has files for run over the files I haven't checked or wants other files, namely in the macros area changed, I can do that. Otherwise, I hopefully have fixed for all of the above comments.

jsturdy commented 5 years ago

Considering not much was changed with this file, I'm inclined to believe this is a problem with the file I'm running over, not the changes made

You can verify by using the unmodified code to run the same analysis on the same file (since it's GE1/1)

dteague commented 5 years ago

I ran the code in the develop branch, and it seems like this code isn't used much because it doesn't work out of the box! (args.filename should be args.infilename. It is fixed in this PR). But the develop branch version gives the exact same results, so this is just a strange fluke of the file I think.

jsturdy commented 5 years ago

I ran the code in the develop branch, and it seems like this code isn't used much because it doesn't work out of the box! (args.filename should be args.infilename. It is fixed in this PR). But the develop branch version gives the exact same results, so this is just a strange fluke of the file I think.

@AndrewLevin there was some issue in the past that was related to this filename vs infilename if I recall correctly, was this resolved? is the change that @dteague is making here counter to that discussion? (@dteague, you can probably search the issues for that string to find what the discussion was about)

dteague commented 5 years ago

Pinging for status Also with #256 , it might be good to fix this concurrently with this PR else GE21 plots can't be graphed (will look into fixes and possibly make another PR)

mexanick commented 5 years ago

@dteague the issue #256 requires a separate PR. Would be great if you can provide it

AndrewLevin commented 5 years ago

I ran the code in the develop branch, and it seems like this code isn't used much because it doesn't work out of the box! (args.filename should be args.infilename. It is fixed in this PR). But the develop branch version gives the exact same results, so this is just a strange fluke of the file I think.

@AndrewLevin there was some issue in the past that was related to this filename vs infilename if I recall correctly, was this resolved? is the change that @dteague is making here counter to that discussion?

I think maybe you are referring to this change

https://github.com/cms-gem-daq-project/gem-plotting-tools/pull/239/files#diff-058f061e69c295595d79241cf963cf20L105-R106

but that has nothing to do with this.

(@dteague, you can probably search the issues for that string to find what the discussion was about)

jsturdy commented 5 years ago

I ran the code in the develop branch, and it seems like this code isn't used much because it doesn't work out of the box! (args.filename should be args.infilename. It is fixed in this PR). But the develop branch version gives the exact same results, so this is just a strange fluke of the file I think.

@AndrewLevin there was some issue in the past that was related to this filename vs infilename if I recall correctly, was this resolved? is the change that @dteague is making here counter to that discussion?

I think maybe you are referring to this change

https://github.com/cms-gem-daq-project/gem-plotting-tools/pull/239/files#diff-058f061e69c295595d79241cf963cf20L105-R106

but that has nothing to do with this.

(@dteague, you can probably search the issues for that string to find what the discussion was about)

OK, thanks @AndrewLevin, I just wanted to make sure that this change was properly motivated and not rehashing something historical.

jsturdy commented 5 years ago

@dteague, sorry, can you rebase this one final time? just want to make sure it would be a good merge as you've touched lots of files

dteague commented 5 years ago

Just took care of that. Hopefully I rebased correctly and all is up to snuff!

lpetre-ulb commented 5 years ago

While testing the new GE2/1 setup at FIT, Stephen and I encountered an issue with the usage of this PR within the GE2/1 compatibility PR in vfatqc-python-tools.

Indeed, in getVFAT3CalInfo the gemType argument is not set in testConnectivity.py nor in any other script; this is an issue for GE2/1 operation. While this seems an issue related to vfatqc-python-tools PR, the design in dbutils.py generalization could be improved to prevent this kind of problem.

In fact, why is the GEM type even required to be given to the DB util functions? I mean, one should be able to request the data for an arbitrary number of VFATs; 24 for GE1/1, 12 for GE2/1 or n for a manual lookup.

I see that gemType is used only here: https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/6612de6cc575eef2b588bc4678c559c80a13d6a5/utils/dbutils.py#L176

Couldn't vfatsPerGemVariant[gemType] be advantageously replaced with len(vfatList)? This would make the DB util functions fully generic without any requirement on the GEM type. As a side effect, the PR in vfatqc-python-scripts would work without modification.

dteague commented 5 years ago

Couldn't vfatsPerGemVariant[gemType] be advantageously replaced with len(vfatList)? This would make the DB util functions fully generic without any requirement on the GEM type. As a side effect, the PR in vfatqc-python-scripts would work without modification.

Very fair, @lpetre-ulb, that is a much better implementation that I didn't see! Let me fix that quickly

lpetre-ulb commented 5 years ago

Thank you for the quick fix @dteague!

jsturdy commented 5 years ago

@mexanick, do we want to hold this while @sbutalla tests at FIT, or just merge it as is and do bugfixes later? oops! too slow of a refresh! (thanks @lpetre-ulb!)

lpetre-ulb commented 5 years ago

@sbutalla and I discovered what seems like a bug yesterday in the DAC scans plots for GE2/1 (at least). The title is a fifth order polynomial and the axis do not have any legend. Stephen, can you open an issue? I don't have access to the plots.