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

Adding metric visualization #342

Merged
merged 35 commits into from
Dec 5, 2022
Merged

Adding metric visualization #342

merged 35 commits into from
Dec 5, 2022

Conversation

sashavor
Copy link
Contributor

@sashavor sashavor commented Nov 7, 2022

I've currently added the code from ComplexRadar() and it does work, but I can't figure out at what point it makes most sense to add the invert_range flag
@lvwerra would love your help!

@sashavor sashavor requested a review from lvwerra November 7, 2022 18:03
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 7, 2022

The documentation is not available anymore as the PR was closed or merged.

@sashavor sashavor marked this pull request as ready for review November 7, 2022 22:20
@sashavor
Copy link
Contributor Author

sashavor commented Nov 7, 2022

@lvwerra lmk what you think now.

Also, where is the best place to document the viz function? in the evaluate docs directly, or via a README?

@sashavor sashavor changed the title WIP: visualizing metrics Adding metric visualization Nov 7, 2022
@NimaBoscarino
Copy link
Contributor

If we plan on building out more visualizations, would it be worth committing to writing this module with the OO-interface for matplotlib? As far as I know it would make testing easier, and I think it would be more dev-friendly for composability + subclassing to extend functionality if folks have custom visualizations that they want to build out.

@lvwerra

@HuggingFaceDocBuilder
Copy link

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Looks really good @sashavor, congrats on mastering unittests 🎉 Left a comments and then I think it's good to go!

src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
@lvwerra
Copy link
Member

lvwerra commented Nov 15, 2022

One thing that's currently missing is documentation. I think it would be great to have some docs on this. E.g. create a new section Vizualization and show how to combine multiple examples from the evaluator runs (like you did in the previous experiments).

@sashavor
Copy link
Contributor Author

Also, I don't understand the CircleCI errors 😬

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Only minor things left to do. Can you use the following format for the docstrings:
https://github.com/huggingface/doc-builder#writing-source-documentation

docs/source/a_quick_tour.mdx Outdated Show resolved Hide resolved
docs/source/a_quick_tour.mdx Outdated Show resolved Hide resolved
docs/source/a_quick_tour.mdx Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
src/evaluate/visualization.py Outdated Show resolved Hide resolved
Sasha Luccioni and others added 2 commits November 25, 2022 11:04
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
Sasha Luccioni and others added 4 commits November 25, 2022 11:39
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
Co-authored-by: Leandro von Werra <lvwerra@users.noreply.github.com>
@lvwerra
Copy link
Member

lvwerra commented Nov 28, 2022

The CI is green now :) I think there are just two more things left to do:

  1. standardize the docstring of ComplexRadar
  2. add a reference to the vizualization tool in docs/source/package_reference. can be a new file.

@sashavor sashavor merged commit 2631887 into main Dec 5, 2022
@sashavor sashavor deleted the viz branch December 5, 2022 15:56
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.

None yet

5 participants