Fix sort by confidence or prediction without postprocessing #327
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
JosephMarinier
commented
Dec 7, 2022
JosephMarinier
commented
Dec 7, 2022
Comment on lines
-113
to
-118
| if ( | ||
| sort_by in {UtterancesSortableColumn.confidence, UtterancesSortableColumn.prediction} | ||
| and pipeline_index is None | ||
| ): | ||
| sort_by = UtterancesSortableColumn.index | ||
|
|
There was a problem hiding this comment.
I combined the check for pipeline_index being None with the other conditions on sort_by, so there is only one place to look at to understand how sort_by works.
JosephMarinier
commented
Dec 7, 2022
| UtterancesSortableColumn.index: DatasetColumn.row_idx, | ||
| UtterancesSortableColumn.utterance: config.columns.text_input, | ||
| UtterancesSortableColumn.label: config.columns.label, | ||
| UtterancesSortableColumn.prediction: DatasetColumn.postprocessed_prediction, |
There was a problem hiding this comment.
I removed this sort_mapping since it's no longer a 1:1 mapping, as it now depends on without_postprocessing.
- Split into unit tests - Assert that responses are sorted
JosephMarinier
changed the title
JosephMarinier
force-pushed
the
joseph/fix-sort-confidence-without-postprocessing
branch
from
e829dab
to
644d754
Compare
Fix sort confidence or prediction without postprocessing while also simplifying the mapping from `UtterancesSortableColumn` to the actual dataset column, since it's no longer 1:1 (it now depends on `without_postprocessing`).
JosephMarinier
changed the title
gabegma
approved these changes
Dec 15, 2022
sethnow
reviewed
Dec 15, 2022
| # The `sort=confidence` is ignored because no pipeline is specified | ||
| resp = client.get("/dataset_splits/eval/utterances?sort=confidence").json() | ||
| first_idx_ignored = resp["utterances"][0]["index"] | ||
| assert first_idx_ignored == first_idx | ||
| assert len(resp["utterances"]) == 42 |
There was a problem hiding this comment.
May be use a global variable for 42 instead of repeats?
There was a problem hiding this comment.
👍 Added one in 7458251
JosephMarinier
deleted the
joseph/fix-sort-confidence-without-postprocessing
branch
JosephMarinier
added a commit
that referenced
this pull request
Dec 20, 2022
* Clean up test utterances - Split into unit tests - Assert that responses are sorted * Test sort without postprocessing * Fix sort without postprocessing - Fix sort confidence or prediction without postprocessing while also simplifying the mapping from `UtterancesSortableColumn` to the actual dataset column, since it's no longer 1:1 (it now depends on `without_postprocessing`). * Polish comment
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.
Resolve #319
Description:
UtterancesSortableColumnto the actual dataset column, since it's no longer 1:1 (it now depends onwithout_postprocessing).Checklist:
You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.
ran
pre-commit run --all-filesat the end.our users.
READMEfiles and our wiki for any big design decisions, if relevant.