Fix segmented custom performance/output metrics #1518
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richard-rogers
commented
May 10, 2024
Comment on lines
+777
to
+778
| self._tag_custom_perf_metrics(file) | ||
| self._tag_custom_output_metrics(file) |
There was a problem hiding this comment.
Suggested change
| self._tag_custom_perf_metrics(file) | |
| self._tag_custom_output_metrics(file) |
I think the tagging happens recursively, so these aren't needed
richard-rogers
commented
May 10, 2024
| classifier="output", data_type=data_type, discreteness=discreteness # type: ignore | ||
| ) | ||
| self._set_column_schema(column_name, column_schema=column_schema) | ||
| def _tag_custom_output_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView, ResultSet]): |
There was a problem hiding this comment.
Suggested change
| def _tag_custom_output_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView, ResultSet]): | |
| def _tag_custom_output_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView]): |
I don't think we need to handle ResultSet here
richard-rogers
commented
May 10, 2024
| if column_name.startswith(perf_col): | ||
| metric = KNOWN_CUSTOM_PERFORMANCE_METRICS[perf_col] | ||
| self.tag_custom_performance_column(column_name, default_metric=metric) | ||
| def _tag_custom_perf_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView, ResultSet]): |
There was a problem hiding this comment.
Suggested change
| def _tag_custom_perf_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView, ResultSet]): | |
| def _tag_custom_perf_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView]): |
richard-rogers
commented
May 10, 2024
| @@ -132,6 +132,26 @@ def _check_whylabs_condition_count_uncompound() -> bool: | |||
| return True | |||
|
|
|||
|
|
|||
| def _get_column_names(x: Union[DatasetProfileView, SegmentedDatasetProfileView, ResultSet]) -> Set[str]: | |||
There was a problem hiding this comment.
I don't think this needs to handle ResultSet
jamie256
reviewed
May 11, 2024
| self._set_column_schema(column_name, column_schema=column_schema) | ||
| def _tag_custom_output_metrics(self, view: Union[DatasetProfileView, SegmentedDatasetProfileView, ResultSet]): | ||
| column_names = _get_column_names(view) | ||
| for column_name in column_names: |
There was a problem hiding this comment.
I guess the only reason we do this is so that we get the correct value for k, otherwise the column names are known.
|
Fixed better in 1.4.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Tag custom performance/output metrics for segmented profiles.
Changes
Fix tagging logic to handle segmented cases
I have reviewed the Guidelines for Contributing and the Code of Conduct.