-
Notifications
You must be signed in to change notification settings - Fork 26
Feature Request: GE2/1 Compatibility for Plotting #226
Comments
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 Then at the start of each routine this should be read from from the Additionally more than just the loop variables would need to change. We would need GE2/1 versions of the following dictionaries: gem-plotting-tools/mapping/chamberInfo.py Lines 37 to 56 in 135f4d8
And gem-plotting-tools/mapping/chamberInfo.py Lines 97 to 107 in 135f4d8
e.g. these dictionaries should have a level of nesting that is added and take 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 This can probably be accomplished by changing all instances of Perhaps the best solution is that 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:
|
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... |
Plan for changesOverviewThis change is to follow the basic outline given by Brian. First, a new metadata to the files called 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
SummaryWith these changes, it will it should keep functionality of all programs in the Let me know if there are things I should expand on and/or problems that might need to be fixed with this plan! |
These are dictionaries; so why aren't we just adding additional outer keys, e.g.
e.g. 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
Why is a function needed? If for vfat in range(vfatsPerGemVariant[gemType]):
do something This replaces all for loop indices; or it's C++
It would be good to see your proposed prototype before submitting a PR and how to refactor various algorithms to use this.
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. |
Plan RevisionBasic ChangesHere 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.pyThe chambers are generalized for all gemTypes available ("ge11", "ge21", "me0"). All the chamber dictionaries ( chamber_vfatPos2PadIdx[gemType] = chamber_vfatPos2PadIdx<Old version> To aid in readability, this could be renamed to be something more descriptive like " These 4 dictionaries are used in the following spots in the code:
So these will have to be updated most immediately to propagate the 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_maxiEtaiPhiPair = {"ge11": (8,3),
"ge21": (4,3), etc.} but this could also be added to the anautilities.pyThere 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 " fitScanData.pyFor the file, SCurve specific changesThe next question in this issue is scope—the issue was to address the function
To generalize these functions, we generalize the functions into makeAxBCanvas ProposalTo 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" ( The code can be found HERE saveSummary ProblemsThere are some problems I found with the code that might need to be fixed with regards to the old
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].
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 The PanPin2 problem can be bypassed (for now) because saveSummary Proposal
Some default behavior might want to be changed. Limitations/ErrorsAs 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 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 ScopeWith 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 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. |
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
But if the acronym is inside the camelCase it should be all caps, i.e.
See my comments on these two below.
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.
Again see my comments below.
If the argument is not necessary do not add it; this clutters the code. Only add it where it is necessary.
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.
Passing gem-plotting-tools/fitting/fitScanData.py Lines 410 to 425 in 62b4d47
Keep in mind that the class and function declared here are also used by files in the
While @sbutalla only mentioned
This doesn't follow my original instructions here, specifically I was hoping for:
This is how I would expect that 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
I'll be honest this
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 command line option exists and is gem-plotting-tools/utils/anaoptions.py Lines 14 to 25 in 62b4d47
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").
See my comments above.
The place for this to be implemented would be in the following PR in cms-gem-daq-project/vfatqc-python-scripts#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: And then simply updating 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
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 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: But again the preferred route would be to get this from a DB View.
It doesn't invalidate things.
Naively I would assume you did not create this or the histograms for
As discussed above a partial re-factoring would not be acceptable. For example consider how to compare scurve results; with your changes does 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:
Are a pretty good guiding star but be careful for locations where 3 iphi sectors were assumed, e.g. gem-plotting-tools/utils/scurveAlgos.py Lines 523 to 525 in 62b4d47
Additionally when summary distributions are created that represent "Observable vs. vfatN If I think of additional comments I will write them. |
Thank you for the detailed comments! I will make these changes and give an update soon as I fix/investigate things:
Noted
I've implemented that HERE. For the
Sorry for the misunderstanding, I will make the changes more to those specifications and update the gists I've posted.
I'll just do some tests, make sure this suspicious code doesn't cause things to crash and minimally fix it.
The code does produce those plots, and it has 12 bins, but they have empty entries 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. |
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 thedetType
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 forgemVariant
and use thevfatsPerGemVariant
to assign the number of VFATs for the propergemType
, and set the default value ofgemType
to GE11 to maintain backwards compatibility: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
135f4d8
bash
The text was updated successfully, but these errors were encountered: