Ge21 generalizing

[dteague]

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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

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.

• anaDACScan.py, anaUltraScurve.py, anaSBitThresh.py, anaUltraLatency.py - work for both
• anaUltraThreshold.py, anaSBitMonitor.py - work for ge11, could not fine ge21 file
• anaSBitReadout.py, anaXDAQLatency.py - could not find input file for either

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[jsturdy]

• extraneous whitespace should be cleaned up
• all debugging print statements should be removed (or added to an appropriate debug block)
• all necessary prints should be python3 compatible, i.e., print() instead of print
• all new string formatting should use "{}".format() rather than "%"() style

I think there are some additional lookup optimizations that can be made, so that map lookups happen only as needed

[jsturdy]

unfortunately, there are two conventions for the "all VFATs summary" plot, one is to put it in index -1, one is to put it in index 24
this should be cleaned up everywhere, but this is outside this PR

[jsturdy]

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

[dteague]

• I changed most all of the print statements to python 3 format
• I changed most all of the % string formatting to the preferred .format() function
• I made a variable for many function for max vfats and max channels (nVFATS and maxChans). - Of course the naming can be changed easily, but it removes multiple look ups of the vfat dictionary and remove some 128 magic numbers from the code

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]

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]

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 😝

[jsturdy]

Here possibly 8-> NUM_IETA_SECTORS
Actually, I think it's the s-bit partition, which is not tied to the ieta sectors

[jsturdy]

Here possibly 9-> NUM_IETA_SECTORS+1, probably best to check the semantics when the Fill is done

[dteague]

Hard to tell, but I don't think. This variable is based on a loop that runs over the clusterNames which are derived from the branches in file, ie

gem-plotting-tools/anaSBitReadout.py

Lines 282 to 295 in c987b20

	for cName in clusterNames:
	# Remove in a later refactoring
	# Right now if an sbit is not sent L1A delay will be max and sbit address is 0x0 and cluster size is 0x0, this is 0x3ffc000; so we ignore this word
	# Otherwise it will always report SBIT 0 of VFAT 7
	word = event[cName]
	if word == 0x3ffc000:
	continue
	# INVALID ADDRESS CHECK
	sbitAddr = ((word) & 0x7FF)
	if sbitAddr >= 1536:
	continue
	nValidClusters+=1

and

gem-plotting-tools/anaSBitReadout.py

Lines 190 to 193 in c987b20

	for branch in inT.GetListOfBranches():
	bNames.append(branch.GetName())
	clusterNames = copy.deepcopy(bNames)
	clusterNames.remove("evtNum")

[jsturdy]

It wasn't my intention with my comment to have you do all the conversions, (just the ones that were in code you added/modified), but it's a good thing to have done anyway :-)
However, here does the formatting stay constant?
I suppose it can guess at the type, just need to make sure there isn't any implicit type conversion happening during the formatting...

[dteague]

I ended up doing a replace for the files I edited, and changed up the numbers when need be! I can add types to these just to be on the safe side

[jsturdy]

A few small checks

[dteague]

This seems like an off by one error, namely the (64-1) and (stripPinOrChan+1) seem to be in opposition, eg
stripPinOrChan = 0 => bin = 62
stripPinOrChan = 63 => bin = -1
Want to confirm before I try and change this

[jsturdy]

I don't know...

• What is the valid range of values for stripPinOrChan?
• What do the current histograms look like (especially if you show the over/underflow stats)
• All things being equal, it looks to me like the +1 should be outside the parentheses, and maybe also vfatHistos should be vfatHistosPanPin2...

[dteague]

I did some tests, and having the +1 outside was correct. stripPinOrChan in {0,...,127} and the code works by if done by PanPin, the channels are split between the two histograms, vfatHistos and vfatHistosPanPin2, each having a domain of [0,63].

All that being said, I made the fix!

[dteague]

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

• anaDACScan.py, anaUltraScurve.py, anaSBitThresh.py, anaUltraLatency.py - work for both
• anaUltraThreshold.py, anaSBitMonitor.py - work for ge11, could not fine ge21 file
• anaSBitReadout.py, anaXDAQLatency.py - could not find input file for either

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]

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]

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]

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]

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]

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

[AndrewLevin]

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]

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]

@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]

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

[jsturdy]

Ready for squash

[lpetre-ulb]

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:

gem-plotting-tools/utils/dbutils.py

Line 176 in 6612de6

{ 'vfatN':[vfat for vfat in range(vfatsPerGemVariant[gemType])],

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]

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]

Thank you for the quick fix @dteague!

[jsturdy]

@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!)

[jsturdy]

now ready for squash

[mexanick]

I think it is fine to merge and start testing. Bug reports/fixes should be provided upon necessity.

[lpetre-ulb]

@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.