Class implementations of faithfulness and extractiveness metrics #323
Conversation
… metrics.py Enable configurable summaCZS model, and configurable input_column.
There was a problem hiding this comment.
LGTM, thanks, very nicely done!
I also have a question regarding whether it could be preferred to pass in models (eg. summac, bert) in the constructor? I followed the existing structure of the BertScore class where we don't pass in the bert instance and there is a comment that says # We only initialize on first compute (reference). I followed the same convention in the faithfulness implementation but wanted to understand why this is preferred over passing in the model directly in the constructor?
Constructors of all these metrics are systematically loaded when initializing Metrics. For the BERTScorer that you linked, this means initializing and loading into memory a BERT model, whether it's actually going to be used or not by the user. Actually loading the model at first use means that the model is only loaded if necessary.
|
You'll need to fix the style :) |
…ventions of other classes. Also applied style fix.
|
Thanks for the review :) Applied style fixes, and modified Extractiveness stats metric initialization similar to Faithfulness and BertScore. |
Refactor extractiveness to be a class and modified its instantiation in metrics.py Refactor faithfulness to be a class and modified its instantiation in metrics.py Enable configurable summaCZS model, and configurable input_column. --------- Co-authored-by: Clémentine Fourrier <[email protected]>
Refactor extractiveness to be a class and modified its instantiation in metrics.py Refactor faithfulness to be a class and modified its instantiation in metrics.py Enable configurable summaCZS model, and configurable input_column. --------- Co-authored-by: Clémentine Fourrier <[email protected]>
This is a refactoring of the faithfulness and extractive functions to separate classes, addressing the TODOs (extractiveness todo and faithfulness todo).
This change was motivated by a need to easily create new instances of these two metrics while setting custom column name as the input to be evaluated against. Currently, input is hardcoded as the "text" column which may not exist or point to the input for different datasets.
I also have a question regarding whether it could be preferred to pass in models (eg. summac, bert) in the constructor? I followed the existing structure of the
BertScoreclass where we don't pass in the bert instance and there is a comment that says# We only initialize on first compute(reference). I followed the same convention in the faithfulness implementation but wanted to understand why this is preferred over passing in the model directly in the constructor? Thanks in advance for the review and feedback :)