-
Notifications
You must be signed in to change notification settings - Fork 211
Onboard text classification inputs to new object #1022
Conversation
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.
Looks good, some comments.
element[DataKeys.TARGET] = targets | ||
def _resolve_target(target_keys: Union[str, List[str]], element: Dict[str, Any]) -> Dict[str, Any]: | ||
if not isinstance(target_keys, List): | ||
element[DataKeys.TARGET] = element.pop(target_keys) |
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.
Would this work as expected ?
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.
This is just a preprocessing step before our own target handling mechanism. So it doesn't matter what comes out here as we will later handle it properly with our classification utilities.
self.load_target_metadata(targets) | ||
|
||
# If we had binary multi-class targets then we also know the labels (column names) | ||
if self.target_mode is TargetMode.MULTI_BINARY and isinstance(target_keys, List): |
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.
Why do you override the state there ? If the state is shared among train, val. This would override the train one.
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.
The classification state is only ever shared from the train data (becuase labels are always inferred from the train data rather than val. or test). In this case, the labels are binary, so we assume that a one in a column corresponds to having that label. So the labels inferred by our classification utils are None
but we replace them with the target keys from the dataset.
class TextClassificationBackboneState(ProcessState): | ||
"""The ``TextClassificationBackboneState`` records the ``backbone`` in use by the ``TextClassifier``.""" | ||
|
||
backbone: str |
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.
Do we want to expend the state to anything behind passed along ?
Codecov Report
@@ Coverage Diff @@
## master #1022 +/- ##
==========================================
- Coverage 87.09% 81.24% -5.86%
==========================================
Files 254 254
Lines 13778 13735 -43
==========================================
- Hits 12000 11159 -841
- Misses 1778 2576 +798
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
What does this PR do?
Part of #964
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃