Coverage refactor, added CoverageMetricSet and CoverageScoreCard #42
Conversation
…e signature for both
…d (get_performance_metrics -> get_coverage_metrics)
There was a problem hiding this comment.
Extremely well done @kajocina. Really nice work - with this in we will release the 0.1.0 version. Currently it has conflicts with the metricset.py. Please take a look!
|
Would also be good to add an annotator for coverage functions, same as for classification functions. I could do this in a later PR :) |
| self.all_users = all_users | ||
| self.all_items = all_items | ||
|
|
||
| def get_coverage_metrics(self, recommendations: List[Tuple]) -> pd.DataFrame: |
There was a problem hiding this comment.
This should override get_performance_metrics instead of making a new function
Hmm but since it has a different interface (like you said, sorry I missed that!) it wouldn't work
There was a problem hiding this comment.
Since this really changes the interface of a score card, perhaps there is a need for an alternate, higher-level abstraction for a score card that only has the generate_report() function
There was a problem hiding this comment.
I was thinking that we could do something like this - keep a parent ScoreCard class and define user-called score cards as children classes of ScoreCard (for ranking, rating, coverage etc.). Should be a slightly cleaner structure. I can do this next.
There was a problem hiding this comment.
I also had considered that the score card class could be merged with the metric set classes, too
rexmex/metrics/coverage.py
Outdated
|
|
||
| def item_coverage(relevant_items: List, recommendations: List[List]) -> float: | ||
|
|
||
| def user_coverage(possible_users_items: List[List], recommendations: List[Tuple]) -> float: |
There was a problem hiding this comment.
can you give a more specific type annotation for possible_users_items? Is this a List[List[int]]?
Same for the recommendations
There was a problem hiding this comment.
I switched it to be a tuple since it always has to be of length 2. Added explicit types now. Users can use integers or strings, depending what they're using in their system.
setup.py
Outdated
| @@ -1,6 +1,6 @@ | |||
| from setuptools import find_packages, setup | |||
|
|
|||
| install_requires = ["numpy", "sklearn", "pandas", "scipy", "scikit-learn"] | |||
| install_requires = ["numpy", "sklearn", "pandas==1.3.5", "scipy", "scikit-learn"] | |||
There was a problem hiding this comment.
I'd suggest <= 1.3.5 since this probably is a bit too restrictive at the moment
I hope the pandas new release things gets cleared up asap
There was a problem hiding this comment.
Thanks, changed to 1.3.4 now :)
Codecov Report
@@ Coverage Diff @@
## main #42 +/- ##
==========================================
- Coverage 99.77% 99.36% -0.42%
==========================================
Files 16 16
Lines 886 942 +56
==========================================
+ Hits 884 936 +52
- Misses 2 6 +4
Continue to review full report at Codecov.
|
Summary
Please provide a high-level summary of the changes for the changes and notes for the reviewers
Changes