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

get-started: move importance into plots #118

Closed
wants to merge 1 commit into from

Conversation

shcheklein
Copy link
Member

Based on @maxagin 's feedback , let's make it symmetrical and explicit that importance.png is also a plot.

@dberenbaum
Copy link

This may be more complicated than @maxagin realized 😅. It looks good in the example, but it may set a bad pattern for users that might not work generally. I might be missing some context, but let me explain my understanding of the situation.

DVCLive logs different types of data in up to 3 different folders: scalars, plots, and images (https://dvc.org/doc/dvclive/get-started#outputs). More confusingly, all of these could be plots.

I don't think putting image files into the plots folder will make sense when the other folders are also populated (it might make sense under images if that folder existed). I would find it confusing to have the same folder contain my json and image files.

It might be better to focus on how those folders should be named in DVCLive, as mentioned in iterative/dvclive#246. Considering that all of these folders can include plots, it's probably best not to reserve the plots name for any specific DVCLive folder.

@dberenbaum
Copy link

If we want to do something like #117, then the other plots will all have separate train and test folders while importance.png will not.

@shcheklein
Copy link
Member Author

@dberenbaum okay, sounds good and good points, iterative/dvclive#246 should be a high priority then, since it's a public convention, right? it's not good that we'll be breaking it so soon.

We can close this for now.

@shcheklein shcheklein closed this May 23, 2022
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.

2 participants