refactor calibration / z-score code #847
Conversation
…ation load gracefully; only show z-score explainer if there are z-scores present
…ding & fail gracefully; factor out Z clacluation
…; fix bug in ThresholdEvaluator where parent constructor wasn't called
resolves #840 |
tests/analyze/test_calibration.py
Outdated
| c = garak.analyze.calibration.Calibration("") | ||
| c.load_calibration() | ||
| assert isinstance(c._data, dict) |
There was a problem hiding this comment.
This seems odd, if the value passed is not a valid file path should this raise an exception?
There was a problem hiding this comment.
I guess an exception should be raised at some level, but nothing that stops execution. Added check that load_calibration() returns None (vs. the number of records loaded)
|
|
||
| def load_calibration( | ||
| self, calibration_filename: Union[str, None] = None | ||
| ) -> Union[None, int]: |
There was a problem hiding this comment.
Why is this method expected to return a value if the value is never checked?
There was a problem hiding this comment.
it now is, in tests, to signal when an exception occurred. i can see hot-swapping of calibrations in the future, so I want load_calibration to be self-contained.
| / "calibration.json" | ||
| ) | ||
| else: | ||
| self.calibration_filename = calibration_path |
There was a problem hiding this comment.
Prefer early input validation of the provided value as an existing path when not None. A lazy load of the data seems reasonable, however verification the value will be consumable seems appropriate.
There was a problem hiding this comment.
added validation (verification deferred to existing code invoked at load time)
Co-authored-by: Jeffrey Martin <[email protected]> Signed-off-by: Leon Derczynski <[email protected]>
There was a problem hiding this comment.
This achieves the initial goal. I am not sold on the idea that the class should defer all evaluation of the file until the runtime retrieval of a z_score however not a critical requirement.
I would prefer to see consumers that instantiate the class guard for an exception and avoid further calls to the class if a valid calibration object is not created. I suspect I am simply not aware of the reason this provides an advantage, and as currently used, I am not seeing value in logging an error on first access of a score vs on create of the object.
On reflection I think this might be a better way to go, will take a look |
…on failed load to simplify logic elsewhere (this function will always have to be able to signal 'not found' anyway)
| 5: "excellent", | ||
| } | ||
|
|
||
| ZSCORE_DEFCON_BOUNDS = [-1, -0.125, 0.125, 1] |
There was a problem hiding this comment.
natural name for a 5-point likert scale indicating recommended degree of panic, esp if one was exposed to War Games as a child. my man j-moss has my back on this
report_digest