feat: Hyperopt schema v0, part 1: Move output feature metrics from feature classes to feature configs. #2759
Conversation
ksbrar
changed the title
ksbrar
changed the title
| @@ -240,7 +237,7 @@ def create_preproc_module(metadata: Dict[str, Any]) -> torch.nn.Module: | |||
|
|
|||
|
|
|||
| class CategoryOutputFeature(CategoryFeatureMixin, OutputFeature): | |||
| metric_functions = {LOSS: None, ACCURACY: None, HITS_AT_K: None} | |||
| metric_functions = CategoryOutputFeatureConfig.get_output_metric_functions() | |||
There was a problem hiding this comment.
What do you think about refactoring this into a static accessor instead of a static variable? It could be defined once at OutputFeature level, like:
@staticmethod
def metric_functions():
return self.get_schema_cls().get_output_metric_functions()
Not necessary, just an idea.
There was a problem hiding this comment.
I like this much better!
There was a problem hiding this comment.
This would be cleaner, but there might be some complexity with how OutputFeature base class shadows the class attribute, with def _setup_metrics(self).
IMO, probably worth refactoring/cleaning this up separately.
|
|
||
| @staticmethod | ||
| def get_output_metric_functions(): | ||
| return {LOSS: None, ACCURACY: None, ROC_AUC: None} |
There was a problem hiding this comment.
I just realized that these are sets, since every key is mapped to None. How about making these set literals?
Code style suggestion, not necessary...
| return {LOSS: None, ACCURACY: None, ROC_AUC: None} | |
| return {LOSS, ACCURACY, ROC_AUC} |
There was a problem hiding this comment.
Hi Kabir, happy to go with this PR as is. There's some additional cleanup that @dantreiman suggested, but this PR as is is still cleaner than what we have today, and we could defer the additional refactoring to later, as to continue with checking in the rest of the hyperopt schema changes, WDYT?
| @@ -240,7 +237,7 @@ def create_preproc_module(metadata: Dict[str, Any]) -> torch.nn.Module: | |||
|
|
|||
|
|
|||
| class CategoryOutputFeature(CategoryFeatureMixin, OutputFeature): | |||
| metric_functions = {LOSS: None, ACCURACY: None, HITS_AT_K: None} | |||
| metric_functions = CategoryOutputFeatureConfig.get_output_metric_functions() | |||
There was a problem hiding this comment.
This would be cleaner, but there might be some complexity with how OutputFeature base class shadows the class attribute, with def _setup_metrics(self).
IMO, probably worth refactoring/cleaning this up separately.
|
@justinxzhao Sounds good, I will proceed with the other changes first and put the set change in a separate refactor! |
This is part 1 of the broken up hyperopt schema PR (#2346).
It moves the definition of a feature's metric function definitions from that feature's class to its config. This seems to make more sense overall and it is necessary to populate the JSON schema for the output feature metric section of the hyperopt config.