use the detName branch in analyses

AndrewLevin commented 5 years ago

A pull request to vfatqc-python-scripts (https://github.com/cms-gem-daq-project/vfatqc-python-scripts/pull/276) adds a detector serial number branch to the output trees from all of the scans. This pull request uses this new branch in the analyses

Description

In the analysis of the DAC scans and the sbitRate scans, in which there are multiple links, the detName branch is used to create the output directory paths.

In the analyses of the scurve scans, threshold scans, and latency scans, the detName branch is copied from the input tree to the output tree.

Ensures backwards compatibility by checking whether there is a detName branch in the input tree before trying to access it.

Also, this fixes several bugs which assumed that all 24 VFATs were present in the input.

Types of changes

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

Motivation and Context

This, together with https://github.com/cms-gem-daq-project/vfatqc-python-scripts/pull/276, are intended to resolve https://github.com/cms-gem-daq-project/vfatqc-python-scripts/issues/273

How Has This Been Tested?

This was tested to analyze an scurve scan, a DAC scan, a threshold scan, a latency scan, and an sbitRate scan.

Screenshots (if appropriate):

Checklist:

[x] 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.

AndrewLevin commented 5 years ago

I found another place where chamber_config was used, which is updated now.

AndrewLevin commented 5 years ago

In the analysis of the sbitThresh scans, the writing of the vfatConfig was duplicated in anaSBitThresh.py and in ana_scans.py:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/ana_scans.py#L117-L137 https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/anaSBitThresh.py#L100-L121

I don't see any reason for this duplication, so I have moved it to the sbitRateAnalysis function in threshAlgos.py.

This is related to the detector name branch because the vfatConfig is written to a directory that includes the detector name.

It is also relevant that in many places we are indexing dictionaries based on the OH key. For example:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/utils/threshAlgos.py#L1218 https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/utils/threshAlgos.py#L1259

and many other places. We may want to change the indexing to be based on the detector name instead. I thought that the OH key is more guaranteed to be unique, so I didn't make this change here.

jsturdy commented 5 years ago

In the analysis of the sbitThresh scans, the writing of the vfatConfig was duplicated in anaSBitThresh.py and in ana_scans.py:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/ana_scans.py#L117-L137 https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/anaSBitThresh.py#L100-L121

I don't see any reason for this duplication, so I have moved it to the sbitRateAnalysis function in threshAlgos.py.

OK, seems OK to me, I have to admit that I'm not familiar with the workings of this codebase enough to understand if there was some historical reason for this. Provided you've tested (at least the likely) use cases and not encountered any issues, then your change is probably good for the long term.

This is related to the detector name branch because the vfatConfig is written to a directory that includes the detector name.

It is also relevant that in many places we are indexing dictionaries based on the OH key. For example:

https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/utils/threshAlgos.py#L1218 https://github.com/cms-gem-daq-project/gem-plotting-tools/blob/develop/utils/threshAlgos.py#L1259

and many other places. We may want to change the indexing to be based on the detector name instead. I thought that the OH key is more guaranteed to be unique, so I didn't make this change here.

If you'd like, feel free to also do this change and see if it causes any hiccups (may as well put it in a separate PR so we can get this one through), again, I have no idea if making this change could have unintended side effects

AndrewLevin commented 5 years ago

Curiosity, do you know why ana_scans for the sbitRateAnalysis doesn't call/execute anaSBitThresh.py?

To me it seems like poor style to have python call bash which then calls python again, so maybe the real question is why does anaSBitThresh.py exist.

run_scans.py does call the other .py scripts while ana_scans.py imports functions. I guess using the importable functions are more useful for multithreading and also we are importing and running some of these analyses in testConnectivity.py.

jsturdy commented 5 years ago

Curiosity, do you know why ana_scans for the sbitRateAnalysis doesn't call/execute anaSBitThresh.py?

To me it seems like poor style to have python call bash which then calls python again, so maybe the real question is why does anaSBitThresh.py exist.

run_scans.py does call the other .py scripts while ana_scans.py imports functions. I guess using the importable functions are more useful for multithreading and also we are importing and running some of these analyses in testConnectivity.py.

I know there was effort put into making more things importable, and I don't know where this ended, but perhaps it more done in the plotting side and not yet in the scan side...

AndrewLevin commented 5 years ago

It is rebased now.

cms-gem-daq-project / gem-plotting-tools