-
Notifications
You must be signed in to change notification settings - Fork 397
Parallel coordinates plot #3105
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
Conversation
content/docs/user-guide/experiment-management/comparing-experiments.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Jorge Orpinel <[email protected]>
content/docs/user-guide/experiment-management/comparing-experiments.md
Outdated
Show resolved
Hide resolved
FYI, we discussed in refinement today that it's probably better to make this a separate subcommand, like Although TBH I don't feel too strongly either way. If I'm already looking at the table and decide it would be easier to view it as a PCP, does it make sense to have to use a different command? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it's pretty hard to read the example images because they are so small. Not sure if there's anything we can do about it 🤔 .
content/docs/user-guide/experiment-management/comparing-experiments.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Dave Berenbaum <[email protected]>
This plot is useful to explore the relationships between the metrics _and_ | ||
params used in experiments. | ||
|
||
 _Parallel Coordinates Plot_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it's pretty hard to read the example images because they are so small. Not sure if there's anything we can do about it 🤔 .
@dberenbaum Do you refer to this GIF?
For the other images, I am able to click and open a full page view (where I am able to read all letters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can open them, but they look small when embedded within the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hacked the plotly layout a bit to increase height and font size (also updated plots to use same examples as the above tables, from example-get-started). Please TAL to latest deployment
Line width, unfortunately, is a need sponsor feature plotly/plotly.js#2573 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Font size was the real concern. Looks better now, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hard to read the example images because they are so small
Do you refer to this GIF?
I do think the GIF could use a smaller example. Maybe just 2 params and a single metric (example). I'd also reduce the number of experiments (7 max) if possible so this sample is short.
The ones in the longer example seem fine to me since you can click on them for closer examination (Still preferably a few less experiments would be ideal IMO, but up to you).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about reducing the number of experiments.
I think that the main usage is when you have many experiments and the plot helps to perceive some patterns more easily than the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on reducing to 2 params 1 metric
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will wait for iterative/dvc#7141 in order to update the plot (to perhaps remove the Experiment column). Leaving the current GIF for now, it that's ok
This is in pretty good shape @daavoo, but I left some specific suggestion that I think would be ideal (6 unresolved comments as of now, starting at #3105 (review)). Not trying to block merge but if we have some time please address those. There's also (from the description): Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you @daavoo
Co-authored-by: Jorge Orpinel <[email protected]>
d1d286e
to
6d5930e
Compare
I think I addressed all comments. About the item in description, I think we will have to wait until it gets merged on core and then update |
…ve/dvc.org into dvc-parallel-coordinates
@jorgeorpinel any updates? can we merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should link to /doc/user-guide/experiment-management/comparing-experiments#parallel-coordinates-plot instead in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgeorpinel any updates? can we merge this?
Sure, it's not my intention to bock this. It's def. mergeable (and already approved).
Thanks again for addressing all the feedback @daavoo
* Add section for parallel coordinates * Add parallel coordinates to ref. Include images * Apply suggestions from code review Co-authored-by: Jorge Orpinel <[email protected]> * Incorporate suggestions in ref * Add link to ref * edit * Apply suggestions from code review Co-authored-by: Dave Berenbaum <[email protected]> * Rename title * Added bigger images * Move PCP section up * ref: copy edits on exp show * ref: copy edit exp show --pcp example * Restyled by prettier (#3138) Co-authored-by: Restyled.io <[email protected]> * Apply suggestions from code review Co-authored-by: Jorge Orpinel <[email protected]> * Add links * Update description * Relink Co-authored-by: Jorge Orpinel <[email protected]> Co-authored-by: Dave Berenbaum <[email protected]> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]>
For iterative/dvc#6933
plots.auto_open
indvc.config
#3101