Bug Report: anaDACScans.py generates KeyError if OH0 not in chamber_config

[bdorney]

Brief summary of issue

anaDACScans.py throws a KeyError if 0 (OH0) is not in the list of keys for chamber_config.

Types of issue

• [X] Bug report (report an issue with the code)
• [ ] Feature request (request for change which adds functionality)

Expected Behavior

This should not throw.

I can imagine two cases:

1. data is taken and OH0 actually existed in the data but for some reason chamber_config was not updated correctly,
2. OH0 is assigned as a default for the case of missing links and exists as a dummy.

In case 1 we might want to still analyze this data and place it in some "uncategorised" location. In case 2 perhaps the GEM Tree format should be updated to not set this default link...

Current Behavior

These lines will generate a KeyError if 0 is not in chamber_config dictionary of chamberInfo.py:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/390a76897576eb0eba97eb8286c644b418891025/anaDACScan.py#L106-L112

Possible Solution (for bugs)

Set the chamber name as follows:

if oh in chamber_config.keys():
    cName = chamber_config[oh]
else
    cName = "unknown"

Then cName is passed to runCommand instead of chamber_config[oh]. Not sure if this is the best solution though.

Context (for feature requests)

Prevents data analysis of DAC scans.

Your Environment

• Version used: 59ce1e4cf86c0c9a949969f2df22958a0b8ed43f
• Shell used: zsh

[bdorney]

A thorough check for additional key errors should be performed during testing, for example another KeyError will be created:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/390a76897576eb0eba97eb8286c644b418891025/anaDACScan.py#L173-L185

Which could be resolved with the same suggestion as shown above.

[AndrewLevin]

Here is the pull request: https://github.com/cms-gem-daq-project/gem-plotting-tools/pull/174

I did not try to use a calFile in $DATA_PATH/unknown/ in the case that the OH number is not in the chamber config and the calFile is not passed, since that seemed a little dangerous, but please let me know if that is desired.

[jsturdy]

• Is this only a problem if OH0 is missing, i.e., missing OH3 doesn't result in some similar fault in some cases?
  • What if there are no entries in the chamber_config? Do you put everything in unknown?
• Should one expect that chamber_config is updated?
• Can one simply check at the time of acquisition that all necessary information is present? Such info could be exported into a pickle file and dumped into the output directory, such that all future analysis will have exactly the same conditions, regardless of whether the acquisition time information changes (which I expect it would, in at least some use cases)

Just a few questions that I hope will help clarify (at least for me) whether the proposed solution is a good idea long term. For me, the underlying issue is not clear yet (based on the info in this PR), though it likely stems from these tools being designed with an extremely specific directory structure being expected. I fear that what is being proposed as a solution here may fix this issue, with the cost of adding complexity. Now is probably a good time to reevaluate the use cases and see if the assumptions are still valid, or whether improving the UX is in the best interests of the project moving forward.

[bdorney]

• Is this only a problem if OH0 is missing, i.e., missing OH3 doesn't result in some similar fault in some cases?

It's specific to OH0 based on the default assignment of the link entry in the stored TTree at time of acquisition:

https://github.com/cms-gem-daq-project/vfatqc-python-scripts/blob/6c3a5eb03b7a54b9bb0b9d3ebec2e66ad1b5feb0/treeStructure.py#L7-L18

gemDacCalTreeStructure inherits from gemGenericTree and link defaults to having a value of 0.

The multi link CTP7 module will fill a DAC word for even masked OH's in the output:

https://github.com/cms-gem-daq-project/ctp7_modules/blob/1df4fc489894f5c1b8010b208f58e8f7267ce8a6/src/calibration_routines.cpp#L1299-L1308

This is unnecessary since the dacWord includes the ohN, vfatN, adcVal, and dacVal in it:

https://github.com/cms-gem-daq-project/ctp7_modules/blob/1df4fc489894f5c1b8010b208f58e8f7267ce8a6/src/calibration_routines.cpp#L1238

The reason for the dacWord being written for unmasked OH's is really due to the xhal method for dacScanMultiLink being a DLL and being forced to take a fixed size C-array as an input/return. If this function was boosted to python and dynamic sized containers could be used this would be a moot point.

So the issue is two fold:

1. link in the output TTree is defaulted to 0, and
2. due to legacy xhal limitations at the time of acquisition the tree is filled for all OH's, even zero'd cases entries as shown in dacScanV3.py Lines 75-87

Boosting to python I think would solve this; and then the ctp7_module wouldn't need to fill the zero entries.

• What if there are no entries in the chamber_config? Do you put everything in unknown?

I think this unknown is a stop gap and doesn't treat the underlying problem outlined above.

• Should one expect that chamber_config is updated?

Until a full DB system for every test stand is setup and can be moved I think it's safe to say for the short term all dictionaries in chamberInfo.py should be expected to be up-to-date.

• Can one simply check at the time of acquisition that all necessary information is present? Such info could be exported into a pickle file and dumped into the output directory, such that all future analysis will have exactly the same conditions, regardless of whether the acquisition time information changes (which I expect it would, in at least some use cases)

Just a few questions that I hope will help clarify (at least for me) whether the proposed solution is a good idea long term. For me, the underlying issue is not clear yet (based on the info in this PR), though it likely stems from these tools being designed with an extremely specific directory structure being expected. I fear that what is being proposed as a solution here may fix this issue, with the cost of adding complexity.

I hope the above clarifies the situation.

Now is probably a good time to reevaluate the use cases and see if the assumptions are still valid, or whether improving the UX is in the best interests of the project moving forward.

I think providing a C++ function in the appropriate HwDevice in cmsgemos and exposing this to python will solve the issue. This would also require a PR to ctp7_modules to update dacScanMultiLink.

[bdorney]

This will be resolved by:

• https://github.com/cms-gem-daq-project/ctp7_modules/pull/82/commits/5c7921c0b175a6a73260429120b59c3a62bbdc26
• https://github.com/cms-gem-daq-project/vfatqc-python-scripts/pull/220/commits/df784a36b21a7200e945e24ff37d8dc0fe138888

When those PR's are merged this issue will be closed.

[bdorney]

Issue is resolved.