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 DataFrames and Numpy Arrays for log_plot() #754

Merged
merged 12 commits into from
Dec 14, 2023

Conversation

mnrozhkov
Copy link
Contributor

@mnrozhkov mnrozhkov commented Dec 12, 2023

This PR is a part of this issue: #750

Changes:

  • add support for Add support for DataFrames and Numpy Arrays in log_plot()
  • implemented a new function for convert datapoints to List[Dict] dvclive/utils.py/convert_datapoints_to_list_of_dicts
  • added new Python dependencies: pandas and numpy
  • added a new argument to the log_plot(): columns: Optional[List[str]] = None, - optional list of column names for data in Numpy Array format

JN to test new feature: log_plots.ipynb

TODO

src/dvclive/live.py Outdated Show resolved Hide resolved
src/dvclive/live.py Outdated Show resolved Hide resolved
src/dvclive/utils.py Outdated Show resolved Hide resolved
src/dvclive/live.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Thanks @mnrozhkov! Very clean and simple. I have just two overall suggestions:

  1. Try to require numpy and pandas as narrowly as possible
  2. Drop columns since I think users can achieve the same without it

@mnrozhkov
Copy link
Contributor Author

Thanks @mnrozhkov! Very clean and simple. I have just two overall suggestions:

  1. Try to require numpy and pandas as narrowly as possible
  2. Drop columns since I think users can achieve the same without it

Thanks @dberenbaum! I resolved all comments. Pls, let me know if there are other places to improve 🙌

src/dvclive/utils.py Outdated Show resolved Hide resolved
@dberenbaum
Copy link
Contributor

Thanks @mnrozhkov! Left one last comment and then I think it's ready to merge. Do you plan to also do the docs PR?

@mnrozhkov
Copy link
Contributor Author

Thanks @mnrozhkov! Left one last comment and then I think it's ready to merge. Do you plan to also do the docs PR?

Yes, I plan to update the docs as well.

src/dvclive/utils.py Outdated Show resolved Hide resolved
tests/test_utils.py Outdated Show resolved Hide resolved
@mnrozhkov mnrozhkov marked this pull request as ready for review December 14, 2023 20:03
Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Thanks @mnrozhkov! Great improvement here.

@dberenbaum dberenbaum merged commit 8fc805d into iterative:main Dec 14, 2023
10 of 11 checks passed
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

3 participants