-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding metric wrapper classes #203
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
========================================
Coverage 99.95% 99.95%
========================================
Files 33 35 +2
Lines 2294 2463 +169
========================================
+ Hits 2293 2462 +169
Misses 1 1 ☔ View full report in Codecov by Sentry. |
638a0ee
to
68963b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, I had a couple minor comments about adding docstrings to the base classes and if the placeholder functions should be no-ops (i.e., just pass) instead of NotImplemented. Up to you how if you want to implement those changes.
Problem & Solution Description (including issue #)
The variety of different metric APIs that are present in qp currently make it challenging to write consistent evaluator stages in RAIL. By introducing wrapper classes we can abstract and unify the API (as much as possible) while making it easier to interface with RAIL evaluator stages.
TODO
Need to add simple unit test coverage for the classes
Include PIT Metric wrapper class.
Code Quality
#pragma: no cover
; in the case of a bugfix, a new test that breaks as a result of the bug has been added