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

fix: use resolve "shared" instead of steps to fix artifacts in grouped density transform #9106

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

jonmmease
Copy link
Contributor

Closes #9078

@@ -24,7 +24,8 @@
"extent": [10, 500],
"counts": true,
"steps": 200,
"as": ["value", "density"]
"as": ["value", "density"],
"resolve": "shared"
Copy link
Member

Choose a reason for hiding this comment

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

Should this one change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the input Vega-Lite spec has:

    {
      "density": "value",
      "bandwidth": 10,
      "groupby": ["Measurement"],
      "extent": [10, 500],
      "counts": true,
      "steps": 200
    }

Since groupby is defined we get "resolve": "shared". "steps": 200 is still there because it's explicitly specified in the Vega-Lite transform.

@jonmmease jonmmease requested a review from domoritz September 20, 2023 01:30
@jonmmease jonmmease merged commit 06e63e6 into main Sep 20, 2023
9 checks passed
@jonmmease jonmmease deleted the jonmmease/gh_9078 branch September 20, 2023 01:36
@joelostblom
Copy link
Contributor

I'm teaching a class starting middle of next week which would benefit from having this included. Is there any chance for a patch release of VL that includes this fix before then? It is not critical as I can work around it by teaching resolve_scale manually or use and older version of Altair, but just wanted to check-in, in case anyone has the bandwidth.

@domoritz
Copy link
Member

Yes. I think either @kanitw and @lsh make a release after #9111 before the weekend or I will make a release with this fix.

@joelostblom
Copy link
Contributor

Awesome, thank you!!

alliefeldman pushed a commit to alliefeldman/vega-lite that referenced this pull request Oct 2, 2023
…d density transform (vega#9106)

* Use resolve shared instead of steps

* Update density tests

* chore: update examples [CI]

---------

Co-authored-by: GitHub Actions Bot <[email protected]>
@domoritz
Copy link
Member

domoritz commented Oct 2, 2023

https://github.com/vega/vega-lite/releases/tag/v5.15.1

@joelostblom
Copy link
Contributor

That's great, thank you so much!

@joelostblom
Copy link
Contributor

Is there a build process that takes some time before the URL https://vega.github.io/schema/vega-lite/v5.15.1.json goes live? Or is there a separate manual step for that?

@domoritz
Copy link
Member

domoritz commented Oct 3, 2023

There is a build process but I got a failure email. @kanitw can you check?

@domoritz
Copy link
Member

domoritz commented Oct 3, 2023

Reran it and worked now.

BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
…d density transform (vega#9106)

* Use resolve shared instead of steps

* Update density tests

* chore: update examples [CI]

---------

Co-authored-by: GitHub Actions Bot <[email protected]>
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.

Grouped density charts have artifacts (again)
3 participants