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

plots: require templates to be JSON and treat them as such #23

Closed
3 tasks
efiop opened this issue Jun 15, 2020 · 3 comments · Fixed by #124
Closed
3 tasks

plots: require templates to be JSON and treat them as such #23

efiop opened this issue Jun 15, 2020 · 3 comments · Fixed by #124
Labels
enhancement New feature or request refactoring

Comments

@efiop
Copy link
Contributor

efiop commented Jun 15, 2020

In our current code, we assume that the template might be in any string format and so we substitute the anchors as strings. We should really start requiring the templates to be JSON, so we could treat them as dicts internally and have a better control over the special vega options. E.g. smoothing from iterative/dvc#3906 , could be handled not with a separate template but a special plot property (iterative/dvc#3906 (comment)).

  • Require templates to be in JSON format
  • handle template contents (Template.content currently might be str or dict) as dicts
  • add plot smoothing option (might be extracted into as separate issue later, depending on complexity)
@efiop efiop added enhancement New feature or request p2-medium refactoring labels Jun 15, 2020
@dmpetrov
Copy link
Member

The initial requirement was related to HTML template support. This is where the strings come from. Later --show-vega requirements become a higher priority.

So, we can easily refuse the HTML\string requirement and move to full JSON. But this might require some minor template redesign.

@pared
Copy link
Contributor

pared commented Jun 15, 2020

TODO:

  • Remove anchors from Template class
  • Replace mechanism of filling anchor from Template._fill_metadata to one working with dict's. funcy.merge might come in hand
  • map existing options and anchors to proper JSON path according to vega-lite schema.

@pared
Copy link
Contributor

pared commented Jun 16, 2020

@dmpetrov noted a good point:
Some of the anchor's paths might not be constant.
Example:
setting title on axis x:
in our default template
encoding/x/title

and for linear sample for vega (not vega-lite) is different:
axes/{scale-corresponding-to-x}/title

@daavoo daavoo transferred this issue from iterative/dvc Mar 31, 2022
@daavoo daavoo removed the p2-medium label Dec 20, 2022
daavoo added a commit that referenced this issue Mar 16, 2023
Prevent unnecessary `dumps`/`loads` calls.

Closes #23
daavoo added a commit that referenced this issue Mar 16, 2023
Prevent unnecessary `dumps`/`loads` calls.

Closes #23
daavoo added a commit that referenced this issue Mar 16, 2023
Prevent unnecessary `dumps`/`loads` calls.

Closes #23
daavoo added a commit that referenced this issue Mar 16, 2023
Prevent unnecessary `dumps`/`loads` calls.

Closes #23
daavoo added a commit that referenced this issue Mar 16, 2023
Prevent unnecessary `dumps`/`loads` calls.

Closes #23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants