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

[WIP] plot metrics: initital #3569

Closed
wants to merge 15 commits into from
Closed

Conversation

pared
Copy link
Contributor

@pared pared commented Apr 1, 2020

  • ❗ I have followed the Contributing to DVC checklist.

  • πŸ“– If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Related to #3409
As of today: only json metrics structured as vega-compliant data accepted (basically list of {"x":, "y":}), due to CORS related issues.

Vega has capabilities of loading the data (and transforming) by itself, but that would require us to set up local server sharing theese files, which seems like overkill, at least in initial version of visualization. So for now we are stuck with injecting display code directly into page code.

@pared pared requested review from efiop and pmrowla April 1, 2020 12:29
dvc/repo/init.py Outdated
@@ -100,6 +101,8 @@ def init(root_dir=os.curdir, no_scm=False, force=False, subdir=False):
with config.edit() as conf:
conf["core"]["no_scm"] = True

init_plot_templates(dvc_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if this is a pre-existing repo and there is no plot template?


class CmdPlot(CmdBase):
def run(self):
self.repo.plot(self.args.targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: It could raise an exception from somewhere, right? Let's wrap it into try&except, same as we do in other places.



def init_plot_templates(dvc_dir):

Copy link
Contributor

Choose a reason for hiding this comment

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

probably a leftover

Suggested change

dvc/plot.py Outdated
def init_plot_templates(dvc_dir):

templates = [DefaultTemplate]
[t(dvc_dir).dump() for t in templates]
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to create this list?

dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
dvc/repo/plot.py Outdated Show resolved Hide resolved
@efiop
Copy link
Contributor

efiop commented Apr 1, 2020

@pared This looks great! πŸ™ Trying it out, I also see that the default resolution is quite crusty, is there a setting for that in the template? Or is it something on my end? Could it show vector graphics by any chance?

@pared
Copy link
Contributor Author

pared commented Apr 1, 2020

plot-sample-2020-04-01-150629

@pared
Copy link
Contributor Author

pared commented Apr 1, 2020

@efiop
take a look at gif provided by me, you can directly edit the template, to meet your needs, basically what you need to know is how to structure Vega json, and adjust template accordingly.

[EDIT]
As of today we use Vega-lite

@pared
Copy link
Contributor Author

pared commented Apr 1, 2020

@efiop

Could it show vector graphics by any chance?

Yes, take note that in the top right corner, there is a button with saving options, you can obtain SVG there.

@pmrowla
Copy link
Contributor

pmrowla commented Apr 2, 2020

@efiop

Could it show vector graphics by any chance?

Yes, take note that in the top right corner, there is a button with saving options, you can obtain SVG there.

I think we may want to use vega's responsive sizing and just make our default template scale automatically to the user's browser window by setting a percentage-based height/width style attribute on the vega div. (I did notice that using 100% for the div width/height makes some of the vega UI stuff run off of the screen, so use something smaller than that)

One other thing I noticed is that when you export to SVG, it generates a graphic with a fixed width/height in pixels so that the exported graphic doesn't actually scale. This always seemed to be the case regardless of whether or not the vega json was using default/fixed/responsive sizing. This can also be fixed by manually editing the SVG to use a percentage-based height/width but I'm wondering if there's a way to make vega do that automatically?

Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

looks good so far, I think we can make the default vega templates a bit nicer though (see my other comment)

@pared
Copy link
Contributor Author

pared commented Apr 2, 2020

I'm wondering if there's a way to make vega do that automatically?

@pmrowla
Thanks for pointing that out.
I'll take note of that. Maybe I missed some settings in template specification. Also, that might be related to the fact that I am using vega-lite currently.

@pared
Copy link
Contributor Author

pared commented Apr 3, 2020

closing in favor of #3577

@pared pared closed this Apr 3, 2020
@pared pared deleted the 3409_metrics branch January 4, 2022 10:12
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.

3 participants