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

Feature Request: GE2/1 Compatibility for Plotting #226

Closed sbutalla closed 5 years ago

sbutalla commented 5 years ago

Brief summary of issue

Currently, the anaUltraScurve.py and related scripts are compatible with GE1/1 only. The GE2/1 has only 12 VFATs per module, while the GE1/1 has 24, so half of the plots are not populated and can therefore be discarded when plotting the final result for GE2/1 data.

Types of issue

Expected Behavior

In order to change this, import the hardware constants from gempython.tools. The user can add an option specifying the --gemType, being either the GE11 or GE21 (ME0 to be implemented later), as well as the detType argument (i.e., long/short for GE11, module number for GE21). After this, the number of VFATs would be assigned to a variable which is used to set the maximum of each of the loops that controls plotting for the individual VFATs.

Current Behavior

The analysis script will create plots for the correct number of VFATs depending on the GEM detector, which will be provided as an option. I.e., for the GE2/1 plot only 12 VFATs per module, and for the GE1/1 plot 24 VFATs.

Possible Solution (for bugs)

Retrieve the hardware constants from hw_constants.py, add option for gemVariant and use the vfatsPerGemVariant to assign the number of VFATs for the proper gemType, and set the default value of gemType to GE11 to maintain backwards compatibility:

from gempython.tools.hw_constants import gemVariants, vfatsPerGemVariant
parser.add_option("--gemType", dest="gemType", type="string", default="ge11", help="String that defines the GEM variant, available from the list: {0}".format(gemVariants.keys()))
parser.add_option("--detType",type="string",help="Detector type within gemType. If gemType is 'ge11' then this should be from list {0}; if gemType is 'ge21' then this should be from list {1}; and if type is 'me0' then this should be from the list {2}".format(gemVariants['ge11'],gemVariants['ge21'],gemVariants['me0']),default=None)

#Assign number of VFATs for gemType
    if args.gemType == "ge11":
        vfatNum = {0}.format(vfatsPerGemVariant)
    elif args.gemType == "ge21":
        vfatNum = {0}.format(vfatsPerGemVariant)
    else:
        print("me0 gemType not currently implemented, exiting.")
        return

For every loop that is plotting distributions, use the maximum range of the loop as vfatNum, i.e.,

for vfat in range(0,vfatNum):

Your Environment

bdorney commented 5 years ago

The plotting tools should not require any additional arguments to handle the different detector types. The better solution is to add additional metadata into the raw data files (defined in vfatqc-python-scripts/utils/treeStructure.py), e.g. TBranches called gemType and detType that contain string data that matches keys/values of the gemVariants dictionary.

Then at the start of each routine this should be read from from the TTree automatically and the routines should determine the index range to use, container size, etc...

Additionally more than just the loop variables would need to change. We would need GE2/1 versions of the following dictionaries:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/135f4d8221333a0c886772414df50ceef2511893/mapping/chamberInfo.py#L37-L56

And

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/135f4d8221333a0c886772414df50ceef2511893/mapping/chamberInfo.py#L97-L107

e.g. these dictionaries should have a level of nesting that is added and take gemType and detType keys to access the correct layout where for ge21 detType's {m1,...,m8} should all map to the same dictionary, e.g.

myGE21mappingDict = {stuff}

for detType in gemVariants["ge21"]:
    chamber_iEta2VFATPos["ge21"][detType] = myGE21mappingDict

Then on the fly the plotting routines should determine whether they should use make3x8Canvas, make2x4Canvas or their GE2/1 equivalents.

This can probably be accomplished by changing all instances of make3x8Canvas and make2x4Canvas to saveSummary and saveSummaryByiEta but this may not be appropriate as in many cases the makeAxBCanvas functions return a TCanvas object while the saveSummary* functions do not return any object.

Perhaps the best solution is that saveSummary and saveSummaryByiEta absorb the functionality of makeAxBCanvas be changed to getSummaryCanvas and getSummaryCanvasByiEta take the gemType and detType arguments then when they are called they determine internally which type of plot should be corrected; following which mapping; and then return the TCanvas object appropriately and only act. There could also be a boolean argument for controlling whether or not the TCanvas is written to disk. This could then replace all calling cases.

Care must be taken and the proposal should be well thought out and clearly outlined before development begins or a PR is submitted. This would include:

bdorney commented 5 years ago

Additionally many distributions are plotted as summaries that have x axis as VFAT number so this would need to be updated as well; containers sizes, etc...

dteague commented 5 years ago

Plan for changes

Overview

This change is to follow the basic outline given by Brian.

First, a new metadata to the files called gemType and detType that contain the basic info to be propagated into the plotting software. A helper function (put in anautilities.py) can be called to set these variables at the beginning of every file. To keep this change as non-destructive as possible, if these TBranches aren't found, they default to "ge11". The geometry info will continue to be stored in mapping/chamberInfo.py, so the code can grab values from this file such as number of iEta, iPhi, VFATs, etc.

For the canvases, the plot can have it's properties set based on the gem and detector type (ie 3x8 plot vs 3x4 plot for ge11 and ge21), so the make#x#Canvas can be absorbed into the saveSummary which would return the canvas and have an option for saving the Summary (possible renaming needed then!)

Files to be changed

Summary

With these changes, it will it should keep functionality of all programs in the ana_scan.py suite completely the same save for functions having different names. This will require a change in the vfatqc-python-scripts, so another issue/PR will need to be made in regards to this, but by putting default values if these branches aren't found, it can be backwards compatible. Following these specifications, it should cause no change for the user and should automatically update plots depending on the gem and detector.

Let me know if there are things I should expand on and/or problems that might need to be fixed with this plan!

bdorney commented 5 years ago

File would be expanded to have dictionaries and those dictionaries set by some setting function called after the import. The actual maps needed by the code, chamber_vfatPos2iEta, chamber_vfatPos2iEtaiPhi, chamber_vfatPos2PadIdx, chamber_iEta2VFATPos will automatically be set to "ge11" specifications so if the setting function is forgotten, it will still default to the old version

These are dictionaries; so why aren't we just adding additional outer keys, e.g.

chamber_vfatPos2iEta["ge11"] = nested dictionary
chamber_vfatPos2iEta["ge21"] = nested dictionary

e.g. chamber_vfatPos2iEta["ge11"] defaults to what we have now. Same idea for all other dictionaries.

Question: do you expect M1...M8 to have different VFAT layout? e.g. is VFAT0 always in the same general location on all detector types? If so then we do not need a detType inner key to gemType in these dictionaries.

A function containing of map for grabbing the number of vfats, iphi, ieta, etc can be created here to remove the magic numbers (namely 24) from code. If the function finds that the branches it needs don't exist, it defaults to the "ge11" specifications

Why is a function needed? If gemType and detType are stored as metadata in the raw data then the dictionaries from hw_constants.py or their boosted equivalents from C++ cmsgemos header files can be used directly, .e.g.

for vfat in range(vfatsPerGemVariant[gemType]):
   do something

This replaces all for loop indices; or it's C++ boosted equivalent from cmsgemos.

Also, the make#x#Canvas functions will be removed and their functionality expanded and put into the saveSummary function which will always return the TCanvas and have a toggle for saving the canvas. This will mean all references to these functions will have to change, but it should be fairly trivial since functionality will be roughly the same save for a slight change in input variables.

It would be good to see your proposed prototype before submitting a PR and how to refactor various algorithms to use this.

All will have to load the gem and detector type and ancillary variables and put them either into global variables or add them as arguments to functions that require them. Most all of the changes in these files are removing magic numbers and replacing them with the values used for each gem and detector type.

It seems like this is not really thoroughly thought through. Again it would be good to see your proposed prototypes before a PR is submitted.

Seems like the proposals here would be API breaking proposals and I think @jsturdy would like a certain direction to be taken here and a timeline for this to be established; I differ to him on this.

dteague commented 5 years ago

Plan Revision

Basic Changes

Here are the basic changes that can be made to the code such that there is no change in the overall code base at all:

chamberInfo.py

The chambers are generalized for all gemTypes available ("ge11", "ge21", "me0"). All the chamber dictionaries (chamber_vfatPos2PadIdx, chamber_vfatPos2iEta, chamber_vfatPos2iEtaiPhi, chamber_iEta2VFATPos) are made into larger dictionaries with the first argument being the gemType, ie

chamber_vfatPos2PadIdx[gemType] = chamber_vfatPos2PadIdx<Old version>

To aid in readability, this could be renamed to be something more descriptive like "chamber_gem2vfatPos2PadIdx" or something. This gets clunky very quickly, so suggestions are welcome!! Right now, the naming convention is kept the same as the non-gemType-dictionaried map. As a side note: all the dictionaries are derived from the chamber_vfatPos2PadIdx as in the old code.

These 4 dictionaries are used in the following spots in the code:

So these will have to be updated most immediately to propagate the chamberInfo.py changes.

Also, the max iEta and iPhi numbers are needed for setting up the canvases as well as looping correctly. This could be found by getting this from the length of chamber_iEta2VFATPos dictionary, but to aid in readability, a new dictionary was created in chamberInfo.py called chamber_maxiEtaiPhiPair which is of the format:

chamber_maxiEtaiPhiPair = {"ge11": (8,3),
                           "ge21": (4,3), etc.}

but this could also be added to the hw_constants.py file if this answer is preferred.

anautilities.py

There are several functions that require information about the number of VFATS and the max iEta for the gem. To start the switch to a more generalized code base, all of the functions that require those 2 pieces of info can just take the gemType as an argument with the default set to "ge11". This would not propagate the change to any part of the code, so the code in its exact form (sans the additions) would work as before.

Functions to add gemType variable in arguments (all in anautilities.py):

Some functions in anautilities.py call other functions in the files, so these functions would have to add gemType to the argument line (not necessary, but allows for ease in future development):

Note: to have gemType passed as an argument to these functions means for the strange addition of "gemType = gemType" in the argument line. It works, but looks very strange.

fitScanData.py

For the file, fitScanData.py, many of the functions have 24 hard coded in, so as in the above cases, gemType is given to these functions and modified to get the correct value. A clean way is to just pass nVFats as an argument to the constructor of the ScanDataFitter and have it be an additional variable in the class so all 24's are replaced with self.nVFats. It would default to 24 for backwards compatibility

SCurve specific changes

The next question in this issue is scope—the issue was to address the function anaUltraScurve.py and if we wish to only change that, we must

  1. Make the following changes to scurveAlgos.py file:

    • change all instances of 24 to vfatsPerGemVariant[gemType]
    • change all instances of 3072 to vfatsPerGemVariant[gemType]*128
    • change all instances of 8 / 9 to len(chamber_iEta2VFATPos) / +1
    • Add gemType=gemType to all anautilities.py function

  2. Alter the code for only the following functions in anautilities (similar changes to above scurveAlgos.py):

    • get2DMapOfDetector()
    • getEmptyPerVFATList()
    • getMapping()
    • parseCalFile()

  3. Fix the canvas making programs. To keep backwards compatible with other parts of the code base, the following functions are made into new generalized functions are created that the scurveAlgos.py calls, but the old versions are left to not break the code base (more on this below):

    • saveSummary()
    • saveSummaryByiEta()
    • make2x4Canvas()
    • make3x8Canvas()

To generalize these functions, we generalize the functions into makeAxBCanvas() and The specifics of these changes are talked about below:

makeAxBCanvas Proposal

To expand on the changes to saveSummary and makeCanvas functions, the makeCanvas function can be generalized, but at the cost of more specification, ie the dimensions of the canvas and a map of the data index to Pad Index must be specified. The code defaults to the dimensions of "ge11" and the data index to pad index map of "ge11" (chamber_vfatPos2PadIdx["gem11"]) as well. This in principle means this new function could be used in all places in the code while staying backwards compatible.

The code can be found HERE

saveSummary Problems

There are some problems I found with the code that might need to be fixed with regards to the old saveSummary functions. These may be my misunderstanding, but nevertheless should be address:

  1. In the saveSummary() function, if the PadPin dictionary isn't null, it divides up the canvas doubly in the phi direction (canv.Divide(8,6)) and fills the canvas with the contents of dictSummary and dictSummaryPanPin2 with the following formula for the PadId: 

    ((ieta+1 + iphi*16)%48 + 16)
((ieta+9 + iphi*16)%48 + 16)

    I haven't tested the code, but it would seem to me that the range of this function would be [16, 55], ie out of the range of the 48 pads. This might be a numbering convention in the Divide function, but from what I see, the range should be [1, 48].

  2. In saveSummaryByiEta() in the Trim line portion of the function, it makes reference to a vfat number, ie:

    for ieta in range(0,maxiEta):
canv.cd(ieta+1)
if trimPt is not None and trimLine is not None:
    ... 
    legend.AddEntry(trimLine, 'trimVCal is %f'%(trimVcal[vfat]))
    ...

    At least in anaUltraScurve.py, this isn't used, but this will likely cause an error. This probably came from a copy and paste from saveSummary() that was never fixed.

The PanPin2 problem can be bypassed (for now) because anaUltraScurve.py only uses PanPin2 if this argument is activated, but there is no commandline argument for doing this, so presumable this is not possible. Also, the trimVCal vfat labeling problem for saveSummaryByiEta() also doesn't seem to be an immediate problem since it isn't used anywhere in the repo (at least now).

saveSummary Proposal

saveSummary() and saveSummaryByiEta() do essentially the same thing but over different ranges, so one change we can make is combining them into one function with a flag isEtaPlot to activate the plot by Eta capability. Also, as suggested by the previous comments, makeCanvas and saveSummary do similar tasks, with saveSummary just adding the ability to add a trimLine and saving the canvas to one name. Again, to combine functionality, the makeAxBCanvas() could act as a helper function with only the new saveSummary being called with an option to save the file if wanted. This implementation is shown HERE

Some default behavior might want to be changed.

Limitations/Errors

As stated in the SaveSummary Problems section, the new code has less functionality in it's current iteration and this might need to be remedied in the PR. At least in my tests, the code works, so these limitations aren't code breaking, but missing in functionality.

Also, because there isn't a branch for the gemType, this is set by hand right now. Also, since this calls for the anaUltraScurve function from the compiled package, this means there is no easy way to implement ge21 capabilities now.

Also, there is no channel mapping (at least that I could find) for ge21, and the code is hard-coded to exit if the mapping isn't long or short. We might want to add the extra mapping for ge21 to the mapping directory and when that's done, the code can be changed to reflect that.

For all of my tests, I've been using the long mapping and the code works. If using the wrong mapping invalidates the tests, then I will need to run over the files with actual mappings.

On that note, there is an issue in my tests with plotting "h2ScurveSigmaDist_All.png", the candle1 draw setting gave some Compute_Integral: Integral = zero because the first vfat bins of this plot were empty. This might be expected, but it was an error to consider in case changes affected the plotting.

Scope

With these changes, only the scurve program would be fixed. Thus comes the question of scope. This process could be expanded to fix the other programs in the gemplotting suite. These files that rely on programs needing gemType and possible more extensive changes are:

If this was to be segmented, the natural break point would be files that rely on saveSummary and makeAxBCanvas, using the merged version used for anaUltraScurve.py in them. These files are the ones bolded. (Note: changes needed are not fully understood yet considering most of this was gleaned from searching these files for mentions of functions in anautilities.py. These will require much testing to correctly fix.)

This issue could turn into a PR for the anaUltraSCurve and naturally expand to encompass these other files, but it will require time to learn and test each of these different pieces of software.

Let me know if there are problems with this plan, changes that should be made, or clarifications needed. At the very least, the code compiles and works as intended to the best of my knowledge.

bdorney commented 5 years ago

To aid in readability, this could be renamed to be something more descriptive like "chamber_gem2vfatPos2PadIdx" or something. This gets clunky very quickly, so suggestions are welcome!! Right now, the naming convention is kept the same as the non-gemType-dictionaried map. As a side note: all the dictionaries are derived from the chamber_vfatPos2PadIdx as in the old code.

Would prefer not changing the name of this dictionary. Also as an aside note the naming convention that @jsturdy prefers is camelCase except for acronyms. If an acronym is at the start of a text string it should be all lower case, e.g see how VFAT and TTC acronyms are handled:

chamber_vfatPos2PadIdx ttcGeneratorState

But if the acronym is inside the camelCase it should be all caps, i.e. stateOfTTCGenerator here TTC is all caps.

  • anautilities.py: make3x8Canvas()
  • anautilities.py: saveSummary()

See my comments on these two below.

Also, the max iEta and iPhi numbers are needed for setting up the canvases as well as looping correctly. This could be found by getting this from the length of chamber_iEta2VFATPos dictionary, but to aid in readability, a new dictionary was created in chamberInfo.py called chamber_maxiEtaiPhiPair which is of the format:

chamber_maxiEtaiPhiPair = {"ge11": (8,3),
                         "ge21": (4,3), etc.}

I strongly advise (read "it would not be allowed") against introducing additional hard coded parameters when it is possible to determine this from already available constants. Consider instead:

chamber_maxiEtaiPhiPair = {}
for gemType in gemVariants.keys():
   chamber_maxiEtaiPhiPair[gemType] = (len(chamber_iEta2VFATPos[gemType]), len(chamber_vfatPos2iEtaiPhi[gemType]/len(chamber_iEta2VFATPos[gemType]))

This would provide you with the information you want but doesn't require any additional hard-coded information.

anautilities.py

There are several functions that require information about the number of VFATS and the max iEta for the gem. ... ...

  • make2x4Canvas()
  • make3x8Canvas() ... ...
  • saveSummary()
  • saveSummaryByiEta()

Again see my comments below.

Some functions in anautilities.py call other functions in the files, so these functions would have to add gemType to the argument line (not necessary, but allows for ease in future development):

If the argument is not necessary do not add it; this clutters the code. Only add it where it is necessary.

Note: to have gemType passed as an argument to these functions means for the strange addition of "gemType = gemType" in the argument line. It works, but looks very strange.

I guess by "argument line" you mean when the function is called. If this is the case then I would say this is not strange, the LHS of that expression is a keyword reference in the calling function and the RHS is the value to be assigned. There's nothing wrong with this.

fitScanData.py

For the file, fitScanData.py, many of the functions have 24 hard coded in, so as in the above cases, gemType is given to these functions and modified to get the correct value. A clean way is to just pass nVFats as an argument to the constructor of the ScanDataFitter and have it be an additional variable in the class so all 24's are replaced with self.nVFats. It would default to 24 for backwards compatibility

Passing nVFATs to the constructor is fine as long as it defaults to 24 but this also needs to be applied to the function defined in this file:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/62b4d47bcb16498774867407bafec8783adc9992/fitting/fitScanData.py#L410-L425

Keep in mind that the class and function declared here are also used by files in the vfatqc repo.

SCurve specific changes

The next question in this issue is scope—the issue was to address the function anaUltraScurve.py and if we wish to only change that, we must

While @sbutalla only mentioned anaUltraScurve.py the issue is really the to ensure compatibility of the entire package. Partial re-factoring would be ill-advised (read "not allowed"). Prefer addressing the entire framework in one go. This could be split across multiple developers but there should be a single PR to the central repo.

makeAxBCanvas Proposal

To expand on the changes to saveSummary and makeCanvas functions, the makeCanvas function can be generalized, but at the cost of more specification, ie the dimensions of the canvas and a map of the data index to Pad Index must be specified. The code defaults to the dimensions of "ge11" and the data index to pad index map of "ge11" (chamber_vfatPos2PadIdx["gem11"]) as well. This in principle means this new function could be used in all places in the code while staying backwards compatible.

This doesn't follow my original instructions here, specifically I was hoping for:

Perhaps the best solution is that saveSummary and saveSummaryByiEta absorb the functionality of makeAxBCanvas be changed to getSummaryCanvas and getSummaryCanvasByiEta take the gemType and detType arguments then when they are called they determine internally which type of plot should be corrected; following which mapping; and then return the TCanvas object appropriately and only act. There could also be a boolean argument for controlling whether or not the TCanvas is written to disk. This could then replace all calling cases.

This is how I would expect that saveSummary, saveSummaryByiEta, make3x8Canvas and make2x4Canvas are handled. Specifically all instances of saveSummary (saveSummaryByiEta) and make3x8Canvas (make2x4Canvas) should be replaced by getSummaryCanvas (getSummaryCanvasByiEta) with the option write2Disk=True, default being False, in the case the original call was saveSummary*. The remainder of the function signature should remain unchanged (since I believe they are both the same since saveSum* calls make*Canvas iirc).

Remember a function that returns something can be called even if you don't set the return value, e.g.

def returnAndPrint(input):
   print(input)
   return input

if __name__ == "__main__":
    returnAndPrint("hi")

This will print "hi" to the screen, so you can call getSummaryCanvas and getSummaryCanvasByiEta without storing their outputs.

saveSummary Problems

There are some problems I found with the code that might need to be fixed with regards to the old saveSummary functions. These may be my misunderstanding, but nevertheless should be address:

  1. In the saveSummary() function, if the PadPin dictionary isn't null, it divides up the canvas doubly in the phi direction (canv.Divide(8,6)) and fills the canvas with the contents of dictSummary and dictSummaryPanPin2 with the following formula for the PadId:
((ieta+1 + iphi*16)%48 + 16)
((ieta+9 + iphi*16)%48 + 16)

I haven't tested the code, but it would seem to me that the range of this function would be [16, 55], ie out of the range of the 48 pads. This might be a numbering convention in the Divide function, but from what I see, the range should be [1, 48].

I'll be honest this PanPin functionality was implemented by a previous student who has since left the project and hasn't really been tested...additionally I never liked this split. I would not worry to much here and open a separate issue for addressing this and proposing a re-factoring on how to better handle PanPin case (or drop entirely).

  1. In saveSummaryByiEta() in the Trim line portion of the function, it makes reference to a vfat number, ie:
for ieta in range(0,maxiEta):
  canv.cd(ieta+1)
  if trimPt is not None and trimLine is not None:
      ... 
      legend.AddEntry(trimLine, 'trimVCal is %f'%(trimVcal[vfat]))
      ...

At least in anaUltraScurve.py, this isn't used, but this will likely cause an error. This probably came from a copy and paste from saveSummary() that was never fixed.

Again old feature by previous student; not really sure if this works as it's not really used and is really only specific to scurves that have been trimmed.

The PanPin2 problem can be bypassed (for now) because anaUltraScurve.py only uses PanPin2 if this argument is activated, but there is no commandline argument for doing this, so presumable this is not possible.

The command line option exists and is -p; it comes as an import from the parent parser:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/62b4d47bcb16498774867407bafec8783adc9992/utils/anaoptions.py#L14-L25

Also, the trimVCal vfat labeling problem for saveSummaryByiEta() also doesn't seem to be an immediate problem since it isn't used anywhere in the repo (at least now).

Indeed; I've removed usage of it since I wasn't sure it would even work. This functionality could probably be removed (read "should be removed").

saveSummary Proposal

See my comments above.

Limitations/Errors

Also, because there isn't a branch for the gemType, this is set by hand right now. Also, since this calls for the anaUltraScurve function from the compiled package, this means there is no easy way to implement ge21 capabilities now.

The place for this to be implemented would be in the following PR in vfatqc (@mexanick):

https://github.com/cms-gem-daq-project/vfatqc-python-scripts/pull/265

This is outside the scope of your (@dteague). But there's nothing to stop you from assuming these branches exist during your development.

For reference (@mexanick) they could be added to the base TTree class:

https://github.com/cms-gem-daq-project/vfatqc-python-scripts/blob/15cf95544f2c2e0d98864ae8a65f166d80f9d245/utils/treeStructure.py#L7-L45

And then simply updating gemGenericTree::setDefaults to:

    def setDefaults(self, options, time):
        """
        Takes as input the options object returned by OptParser.parse_args()
        see: https://docs.python.org/2/library/optparse.html

        Sets values common to all scan scripts (see qcoptions.py)
        """

        self.detType[0] = options.detType
        self.gemType[0] = options.gemType
        self.link[0] = options.gtx
        self.Nev[0] = options.nevts
        self.shelf[0] = options.shelf
        self.slot[0] = options.slot
        self.utime[0] = time

        return

Since @mexanick has implemented command line options for detType and gemType that default to respectively short and ge11 this would then be inserted into the raw TTree's used for analysis.

Also, there is no channel mapping (at least that I could find) for ge21, and the code is hard-coded to exit if the mapping isn't long or short. We might want to add the extra mapping for ge21 to the mapping directory and when that's done, the code can be changed to reflect that.

This I would handle in a separate PR that expands the DB interaction. Just assume the mapping you get is appropriate to the number of VFATs you have. You can use the --extMapping option. You should take up with the Phase II Electronics group to generate an EDMS document that specifies the mapping for GE2/1 similar to the one for GE1/1:

https://edms.cern.ch/ui/#!master/navigator/document?P:1115513923:100195479:subDocs

For short term you can use @lmoureaux's old PR to generate a mapping text file:

https://github.com/cms-gem-daq-project/gem-plotting-tools/pull/134

But again the preferred route would be to get this from a DB View.

For all of my tests, I've been using the long mapping and the code works. If using the wrong mapping invalidates the tests, then I will need to run over the files with actual mappings.

It doesn't invalidate things.

On that note, there is an issue in my tests with plotting "h2ScurveSigmaDist_All.png", the candle1 draw setting gave some Compute_Integral: Integral = zero because the first vfat bins of this plot were empty. This might be expected, but it was an error to consider in case changes affected the plotting.

Naively I would assume you did not create this or the histograms for h2ScurveMeanDist_All.png and h2ScurveEffPedDist_All.png correctly...if you correctly handled the VFAT number then there should have not been empty bins, but less bins, e.g. 12 vs 24 bins for GE2/1 vs. GE1/1. Please dobule check.

Scope

With these changes, only the scurve program would be fixed.

As discussed above a partial re-factoring would not be acceptable. For example consider how to compare scurve results; with your changes does plotScurveFitResults.py work? What about ana_scans.py scurve [args]? I suspect no...

I think you have a good strategy going forward but I would encourage you to look at other functions and tools as well. Statements like these:

Make the following changes to scurveAlgos.py file:

  • change all instances of 24 to vfatsPerGemVariant[gemType]
  • change all instances of 3072 to vfatsPerGemVariant[gemType]*128
  • change all instances of 8 / 9 to len(chamber_iEta2VFATPos) / +1 Add gemType=gemType to all anautilities.py function

Are a pretty good guiding star but be careful for locations where 3 iphi sectors were assumed, e.g.

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/62b4d47bcb16498774867407bafec8783adc9992/utils/scurveAlgos.py#L523-L525

Additionally when summary distributions are created that represent "Observable vs. vfatNensure that the x-axis reflects thevfatsPerGemVariant[gemType]` and not a hangover of 24 as a max on the x-axis.

If I think of additional comments I will write them.

dteague commented 5 years ago

Thank you for the detailed comments! I will make these changes and give an update soon as I fix/investigate things:

Also as an aside note the naming convention that @jsturdy prefers is camelCase except for acronyms. If an acronym is at the start of a text string it should be all lower case, e.g see how VFAT and TTC acronyms are handled:

If the argument is not necessary do not add it; this clutters the code. Only add it where it is necessary.

Keep in mind that the class and function declared here are also used by files in the vfatqc repo. (in ref to fitScanData.py)

Prefer addressing the entire framework in one go.

Indeed; I've removed usage of it since I wasn't sure it would even work. This functionality could probably be removed (read "should be removed").

This is outside the scope of your (@dteague). But there's nothing to stop you from assuming these branches exist during your development.

For short term you can use @lmoureaux's old PR to generate a mapping text file:

Are a pretty good guiding star but be careful for locations where 3 iphi sectors were assumed, e.g.

Noted

I strongly advise (read "it would not be allowed") against introducing additional hard coded parameters when it is possible to determine this from already available constants.

I've implemented that HERE. For the chamber_vfatPos2PadIdx, I've implemented a little algorithm: it works, but there is probably a more efficient solution. This can be revisited.

This doesn't follow my original instructions here, specifically I was hoping for:

Sorry for the misunderstanding, I will make the changes more to those specifications and update the gists I've posted.

I'll be honest this PanPin functionality was implemented by a previous student who has since left the project and hasn't really been tested...additionally I never liked this split. I would not worry to much here and open a separate issue for addressing this and proposing a re-factoring on how to better handle PanPin case (or drop entirely).

I'll just do some tests, make sure this suspicious code doesn't cause things to crash and minimally fix it.

Naively I would assume you did not create this or the histograms for h2ScurveMeanDist_All.png and h2ScurveEffPedDist_All.png correctly...if you correctly handled the VFAT number then there should have not been empty bins, but less bins, e.g. 12 vs 24 bins for GE2/1 vs. GE1/1. Please dobule check.

The code does produce those plots, and it has 12 bins, but they have empty entries Here is the graph.

It has the right format, but the first two bins are empty and gives a repeated version of the error below. Drawing this specific graph without the "candle1" option (say "colz"), it doesn't give the following error.

Error in <TH1D::ComputeIntegral>: Integral = zero

I will investigate this more and hopefully find the root of this problem.

I will make the changes Noted above and update this issue as the code changes become more mature and expand to the rest of the repo. I can also give more detailed comparisons of the old and new output for validation purposes.