search

iterative / example-repos-dev

Source code and generator scripts for example DVC projects
https://dvc.org/doc
21 stars 13 forks source link

get-started: move importance into plots #118

Closed shcheklein closed 2 years ago

shcheklein commented 2 years ago

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

dberenbaum commented 2 years ago

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 https://github.com/iterative/dvclive/issues/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 commented 2 years ago

If we want to do something like https://github.com/iterative/example-repos-dev/pull/117, then the other plots will all have separate train and test folders while importance.png will not.

shcheklein commented 2 years ago

@dberenbaum okay, sounds good and good points, https://github.com/iterative/dvclive/issues/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.