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

Ambiguity measure #361

Merged
merged 17 commits into from
Dec 8, 2022
Merged

Ambiguity measure #361

merged 17 commits into from
Dec 8, 2022

Conversation

jelledouwe
Copy link
Collaborator

This PR contains the AmbiguityMeasure tool for human robot interaction. It is related to one of our recent publications. We also created a colab.

Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

Thank you, being an utility I think what you did is sufficient. I have however a couple of comments:

  • you have created a test (and locally it worked for me) but currently the tool is not being tested in the CI (you need to add it to the .yml files, both master and develop)
  • I believe the the intent is for this tool to be distributed separately (like we do for the hyperparameter tuner), in which case you also need to add it to packages.txt so that one is created for it
  • add a changelog entry

docs/reference/ambiguity_measure.md Outdated Show resolved Hide resolved
docs/reference/ambiguity_measure.md Outdated Show resolved Hide resolved
docs/reference/ambiguity_measure.md Outdated Show resolved Hide resolved
docs/reference/ambiguity_measure.md Outdated Show resolved Hide resolved
docs/reference/ambiguity_measure.md Outdated Show resolved Hide resolved
src/opendr/utils/ambiguity_measure/__init__.py Outdated Show resolved Hide resolved
jelledouwe and others added 5 commits December 8, 2022 11:13
@jelledouwe
Copy link
Collaborator Author

Thank you, being an utility I think what you did is sufficient. I have however a couple of comments:

* you have created a test (and locally it worked for me) but currently the tool is not being tested in the CI (you need to add it to the .yml files, both master and develop)

* I believe the the intent is for this tool to be distributed separately (like we do for the hyperparameter tuner), in which case you also need to add it to packages.txt so that one is created for it

* add a changelog entry

Hi Daniel, thanks for the review! I believe the test is currently already tested in the CI. At least, it appears in the actions on this branch under the util tests. I added the entry to the package list and the changelog.

@jelledouwe jelledouwe requested a review from ad-daniel December 8, 2022 10:36
ad-daniel
ad-daniel previously approved these changes Dec 8, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

Yes you're right, it's being run in the utils job, I didn't see it. Thank you!

tsampazk
tsampazk previously approved these changes Dec 8, 2022
Copy link
Collaborator

@tsampazk tsampazk left a comment

Choose a reason for hiding this comment

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

Changes look good to me, thank you!

@ad-daniel ad-daniel merged commit 423fcd6 into develop Dec 8, 2022
@ad-daniel ad-daniel deleted the ambiguity_measure branch December 8, 2022 12:41
lucamarchionni pushed a commit to lucamarchionni/opendr that referenced this pull request Jun 10, 2024
* Create docs, tests and tutorial for ambiguity measure

* update tutorial

* Add link to colab tutorial

* update style

* add ambiguity measure

* Apply suggestions from code review

Co-authored-by: ad-daniel <44834743+ad-daniel@users.noreply.github.com>

* add ambiguity measure to package list and changelog

* Apply suggestions from code review

Co-authored-by: ad-daniel <44834743+ad-daniel@users.noreply.github.com>

Co-authored-by: ad-daniel <44834743+ad-daniel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants