Skip to content

Move confidence threshold out of /dataset_info#61

Merged
JosephMarinier merged 4 commits intodevfrom
joseph/confidence-threshold
Jun 16, 2022
Merged

Move confidence threshold out of /dataset_info#61
JosephMarinier merged 4 commits intodevfrom
joseph/confidence-threshold

Conversation

@JosephMarinier
Copy link
Copy Markdown
Contributor

@JosephMarinier JosephMarinier commented May 4, 2022

Description:

The idea is to remove pipeline-specific info from /dataset_info.

Checklist:

You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge
it.

  • PRE-COMMIT. You ran pre-commit on all commits, or else, you
    ran pre-commit run --all-files at the end.
  • FRONTEND TYPES. Regenerate the front-ent types if you played with types and routes.
    Run cd webapp && yarn types while the back-end is running.
  • USER CHANGES. The changes are added to CHANGELOG.md and the documentation, if they impact
    our users.
  • DEV CHANGES.
    • Update the documentation if this PR changes how to develop/launch on the app.
    • Update the README files and our wiki for any big design decisions, if relevant.
    • Add unit tests, docstrings, typing and comments for complex sections.

@JosephMarinier JosephMarinier requested a review from Dref360 May 4, 2022 20:39
@JosephMarinier JosephMarinier self-assigned this May 4, 2022
Copy link
Copy Markdown
Contributor

@Dref360 Dref360 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread webapp/src/types/generated/generatedTypes.ts Outdated
@JosephMarinier JosephMarinier force-pushed the joseph/confidence-threshold branch from 2d9e21f to 848cddd Compare May 26, 2022 14:39
@JosephMarinier JosephMarinier changed the base branch from main to dev May 26, 2022 18:59
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dref360, could you advise on the "two approaches" I mentioned in the PR description? Are both OK?

  • I tried two approaches to get some feedback:
    - For /utterances, I get the confidence threshold in the router.
    - For /bins, I get the confidence threshold in the module. I have some trouble with pipeline_index. Before investing more time, is that even a sensible approach?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to me it makes sense to return the threshold in a router OR in a Module depending on the usecase. The way you did it is great!

@gabegma
Copy link
Copy Markdown
Contributor

gabegma commented Jun 10, 2022

Can we merge this?

@JosephMarinier JosephMarinier marked this pull request as ready for review June 10, 2022 14:48
and into `/bins` and `/utterances`. The idea is to remove pipeline-specific info from `/dataset_info`.
@JosephMarinier JosephMarinier force-pushed the joseph/confidence-threshold branch from 848cddd to 385511b Compare June 10, 2022 16:04
@JosephMarinier JosephMarinier enabled auto-merge (squash) June 10, 2022 16:05
@JosephMarinier JosephMarinier disabled auto-merge June 10, 2022 16:22
@JosephMarinier JosephMarinier force-pushed the joseph/confidence-threshold branch from 4f3ab37 to 0684415 Compare June 16, 2022 21:56
@JosephMarinier JosephMarinier enabled auto-merge (squash) June 16, 2022 21:57
@JosephMarinier JosephMarinier merged commit 071841f into dev Jun 16, 2022
@JosephMarinier JosephMarinier deleted the joseph/confidence-threshold branch June 16, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants