Migrating Analysis Tools from vfatqc-python-scripts to gem-plotting-tools #4

bdorney · 2017-07-19T16:31:27Z

Addressing:

#3
cms-gem-daq-project/vfatqc-python-scripts#99
cms-gem-daq-project/vfatqc-python-scripts#104

Also addressing:

cms-gem-daq-project/vfatqc-python-scripts#98

Also built on:

cms-gem-daq-project/vfatqc-python-scripts#100

Tested:

http://cmsonline.cern.ch/cms-elog/998496

Requires joint PR:

cms-gem-daq-project/vfatqc-python-scripts#105

User should have following $SHELL variables declared:

$BUILD_HOME
$DATA_PATH
$ELOG_PATH

Then execute:

setup/paths.sh

Then you'll be ready to go.

Description of how to use ana_scans.py to analyze scan data is found:

https://twiki.cern.ch/twiki/bin/view/CMS/GEMDOCDoc#How_to_Produce_Scan_Plots

Note ana_scans.py was tested separately on a clone of $DATA_PATH on the gemdqm machine. However I didn't make an E-Log documenting this (rookie mistake!).

Requesting review from @cbravo135 and @mexanick

cbravo135

anaUltraScurve was not merged properly.

cbravo135 · 2017-07-20T17:01:44Z

anaUltraScurve.py

+ fitENC = []
+ strList = []
+ masks.append([])
+ for ch in range (0, 128):


This was not correctly merged. This should now be a loop over strips and also lookup_table should be changed to chToStrLUT.

I didn't understand why lookup_table was seemingly split into:
chToStrLUT = [] strToChLUT = []

Looking at the code it wasn't clear why this was done. Test outputs I ran are attached. To me it seems like this implementation is "okay."

fitSums x-axis is labeled Strips but the resulting output is in channel units this way. I do things for a reason, one of the two LUT replaces lookup_table and the other is new so I could make fitSums in strip units easily.

The resulting x-axis is the same in both cases...if it's a concern over axis labeling then I would prefer the implementation shown in anaUltraThresholds.py:

https://github.com/bdorney/gem-plotting-tools/blob/master/anaUltraThreshold.py

Lines 81-119

If I understand this correctly it makes the output plots x-axis based on the three user input options automatically:
->strips
->vfat ch
->panasonic pins

Might be good to unify this.

Not true, look at my code harder. The code this way will definitely be in channel units.

This first version of fitSums is only in strip units, I will add other options when I clean up the code that takes care of switching between the three cases.

Look at the output plot, the distributions are following each other. The x-axis labels may be wrong but the value plotted in fitSummary appears to follow Summary. For example look at VFAT0, the spike matches in the two positions. Also VFAT3 and VFAT4 match pretty well. The rest appear to also.

I doubt this is just a coincidence. Again it seems like the problem is the label on the x-axis

that is right, strList fixes it, I forgot I did that. We should add panList and chList so we can control which we pass to build the TGraphErrors and can control the units of the x-axis then. As the code is now, it will always be in strip units.

Might be better to have one variable here instead of strList which gets passed to the TGraphErrors constructor which is filled depending on the users choice.

cbravo135 · 2017-07-21T07:31:25Z

anaUltraScurve.py

+for i, line in enumerate(intext):
+ if i == 0: continue
+ mapping = line.rsplit('\t')
+ #chToStrLUT[int(mapping[0])][int(mapping[2]) - 1] = int(mapping[1])


Please use my more complete set of LUTs.

Again, would prefer the implementation shown in anaUltraThresholds.py:

https://github.com/bdorney/gem-plotting-tools/blob/master/anaUltraThreshold.py

Lines 81-119 to automatically ensure the correct mapping & x-axis label.

can we please change the name of lookup_table to chToStrLUT because it is more descriptive. lookup_table is a terrible variable name. I might as well call all my integers int1 int2 int3...

Fair point, will do, but I will use "strip" to avoid confusion with "string"

lmoureaux · 2017-07-21T13:42:45Z

anaUltraScurve.py

+for vfat in fitSums.keys():
+ r.gStyle.SetOptStat(0)
+ canv.cd(vfat+1)
+ fitSums[vfat].Draw('ap')


Crashes here when not using the -f option:

Traceback (most recent call last): File "/home/louis/Documents/CERN Summer School/GEM/gem-plotting-tools/anaUltraScurve.py", line 367, in <module> for vfat in fitSums.keys(): NameError: name 'fitSums' is not defined

At @cbravo135 is there are usage case where we would not want to use the option -f this seems like the primary use case...

...nevermind I'll just patch it, simple fix.

It is an easy fix anyways right? Just put the if protection like the rest of the blocks for the fitting option. The idea is without -f it only makes the Summary plot and runs faster since it is skipping all the fitting part. This plot doesn't make sense unless you do the fits, I just forgot to put the if part to protect it.

yes in fact I've already patched it, just making some updates to anaUltraLatency so it will make summary plot as a 8x3 grid

cbravo135 · 2017-07-21T15:29:04Z

anaUltraScurve.py

- canv.cd(vfat+1)
- fitSums[vfat].Draw('ap')
- canv.Update()
+ for vfat in fitSums.keys():


no reason to have .keys() here, that is what python uses by default when you iterate over a dict.

cbravo135 · 2017-07-21T15:32:11Z

anaUltraLatency.py

@@ -83,4 +83,3 @@




Looks like you still haven't added the 8x3 summary plot yet, so I will wait to approve.

Brian Dorney and others added 7 commits July 12, 2017 20:08

renaming latency analysis script

0e609c3

Delete LatencyScanData.py

8d6550a

redesign of repo

02d0a5f

Merge branch 'master' of ssh://github.com/bdorney/gem-plotting-tools

4092d52

Delete anaUltraLatency.py

8fca8d4

added analysis link to DOC twiki page in README.md

8507f26

adding changes by Cameron for VCAL -> fC in SCurve ana

db4a62c

cbravo135 suggested changes Jul 20, 2017

View reviewed changes

cbravo135 suggested changes Jul 21, 2017

View reviewed changes

More descriptive variable names in anaUltraScurve

96ea206

lmoureaux reviewed Jul 21, 2017

View reviewed changes

Patch for anaUltraScurve

b561c4c

cbravo135 reviewed Jul 21, 2017

View reviewed changes

anaUltraLatency.py

@@ -83,4 +83,3 @@

Copy link

Contributor

cbravo135 Jul 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you still haven't added the 8x3 summary plot yet, so I will wait to approve.

cbravo135 approved these changes Jul 21, 2017

View reviewed changes

cbravo135 merged commit 79d312e into cms-gem-daq-project:master Jul 21, 2017

This was referenced Jul 21, 2017

anaUltraThresholds.py crashes when using the --vfatmask flag cms-gem-daq-project/vfatqc-python-scripts#97

Closed

Feature Request: Analysis Analog to run_scans.py cms-gem-daq-project/vfatqc-python-scripts#99

Closed

lmoureaux mentioned this pull request Jul 20, 2018

Relicensing as MIT (was GPLv3) #142

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating Analysis Tools from vfatqc-python-scripts to gem-plotting-tools #4

Migrating Analysis Tools from vfatqc-python-scripts to gem-plotting-tools #4

bdorney commented Jul 19, 2017 •

edited

Loading

cbravo135 left a comment

cbravo135 Jul 20, 2017

bdorney Jul 21, 2017

cbravo135 Jul 21, 2017

bdorney Jul 21, 2017 •

edited

Loading

cbravo135 Jul 21, 2017

bdorney Jul 21, 2017

cbravo135 Jul 21, 2017

cbravo135 Jul 21, 2017

cbravo135 Jul 21, 2017

bdorney Jul 21, 2017

cbravo135 Jul 21, 2017

bdorney Jul 21, 2017

lmoureaux Jul 21, 2017 •

edited

Loading

bdorney Jul 21, 2017 •

edited

Loading

cbravo135 Jul 21, 2017

bdorney Jul 21, 2017

cbravo135 Jul 21, 2017

cbravo135 Jul 21, 2017

Migrating Analysis Tools from vfatqc-python-scripts to gem-plotting-tools #4

Migrating Analysis Tools from vfatqc-python-scripts to gem-plotting-tools #4

Conversation

bdorney commented Jul 19, 2017 • edited Loading

cbravo135 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdorney Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmoureaux Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

bdorney Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdorney commented Jul 19, 2017 •

edited

Loading

bdorney Jul 21, 2017 •

edited

Loading

lmoureaux Jul 21, 2017 •

edited

Loading

bdorney Jul 21, 2017 •

edited

Loading