Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for persistent ID and filtering on ID in exploration space #301

Merged
merged 8 commits into from
Dec 6, 2022

Conversation

gabegma
Copy link
Contributor

@gabegma gabegma commented Nov 24, 2022

Supports #297 and fixes #200

Description:

You can look at the commits' message for the details of what I have done.

Checklist:

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

  • ISSUE NUMBER. You linked the issue number (Ex: Resolve #XXX).
  • PRE-COMMIT. You ran pre-commit on all commits, or else, you
    ran pre-commit run --all-files at the end.
  • 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.

@gabegma gabegma changed the title Ggm/filter Add support for persistent ID and filtering on ID in exploration space Nov 25, 2022
@gabegma gabegma marked this pull request as ready for review November 29, 2022 21:22
@JosephMarinier JosephMarinier linked an issue Nov 30, 2022 that may be closed by this pull request
Copy link
Contributor

@JosephMarinier JosephMarinier left a comment

Choose a reason for hiding this comment

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

You mentioned by was not a great variable name... I agree. It's just a detail, but here is a simple change:

azimuth/utils/dataset_operations.py Outdated Show resolved Hide resolved
azimuth/utils/dataset_operations.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JosephMarinier JosephMarinier left a comment

Choose a reason for hiding this comment

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

You mentioned by was not a great variable name... I agree! It's just a detail, but here is a simple change:

Copy link
Contributor

@JosephMarinier JosephMarinier left a comment

Choose a reason for hiding this comment

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

That's pretty cool! Thank you!
It should then be very easy to add it to the response of /utterances... In another PR?

@gabegma
Copy link
Contributor Author

gabegma commented Dec 6, 2022

It should then be very easy to add it to the response of /utterances... In another PR?

Right @JosephMarinier, maybe it can be included in the PR where you show it on hover in the UI? That way you can name the variable in the API how you prefer. I can also include it in this PR, let me know.

@JosephMarinier
Copy link
Contributor

Perfect plan! 👌

@gabegma gabegma merged commit abad9ed into main Dec 6, 2022
@gabegma gabegma deleted the ggm/filter-id branch December 6, 2022 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants