Reorganize DAC analysis exception classes

AndrewLevin commented 4 years ago

A new exception class DACAnalysisException is introduced which combines VFATDACBiasCannotBeReached and VFATDACFitLargeChisquare.

Description

The new exception class contains two bools which identify which problem or problems cause the exception, and a message with the details.

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

In the case of both a bad bias and a bad fit exception in the same DAC scan, only one of the two exceptions would be thrown, arbitrarily.

How Has This Been Tested?

I have reanalyzed a DAC scan which has both types of exceptions.

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 4 years ago

It looks like you've changed VFATDACFitLargeChisquare to DACAnalysisException, but I don't see any downstream changes that were catching the old exception having been changed. Is this breaking in that regard? Does code need to be modified to work with this change if it is merged in?

Yes, there is code that catches this exception in testConnectivity.py. I will prepare a pull request for vfatqc-python-scripts.

mexanick commented 4 years ago

@AndrewLevin is this stale?

AndrewLevin commented 4 years ago

no, it is not stale

lpetre-ulb commented 4 years ago

@AndrewLevin, is this PR still relevant? In that case, a twin PR should be provided in vfatqc-python-scripts.

lpetre-ulb commented 4 years ago

@AndrewLevin, can we close this PR? Or will the remaining work ever be carried on?

AndrewLevin commented 4 years ago

the work is already carried out actually:

https://github.com/AndrewLevin/vfatqc-python-scripts/commit/d7b24bbe4c74b59a8ad970a28aac03181252a120

I will test it tomorrow

lpetre-ulb commented 4 years ago

@AndrewLevin, have you checked that the changes are thorough and thoroughly tested?

AndrewLevin commented 4 years ago

well, I have did some tests, but there are so many dependencies it is hard to know if I missed something (which is why this should done by gitlab CI instead of a human...)

I think it is unlikely that there is an inconsistency now (anyway it is an exception, and would just not be caught), but it is fine with me if you want to not merge it

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