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

Split distribution chart in 2. Fix colours #1388

Merged
merged 7 commits into from
Nov 19, 2022
Merged

Conversation

Hazelfire
Copy link
Collaborator

@Hazelfire Hazelfire commented Nov 15, 2022

This PR creates a separate class for Multi charts and normal charts. This allows me to differentiate between plot objects with one element and simple distributions, which I think should be rendered differently. Particularly as it comes to colour schemes.

I also fix the colours up a bit for multiplots, they look like this currently:
image

This paves the way to having summary tables for multiplot distributions.

This currently has a lot of duplication, I'll look to remove as much duplication as possible before saying this is ready for review.

@vercel
Copy link

vercel bot commented Nov 15, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
squiggle-components ✅ Ready (Inspect) Visit Preview Nov 19, 2022 at 2:45PM (UTC)
squiggle-website ✅ Ready (Inspect) Visit Preview Nov 19, 2022 at 2:45PM (UTC)

@Hazelfire
Copy link
Collaborator Author

Hazelfire commented Nov 16, 2022

Ok, fixed the duplication.

A comment is that I did this refactor mainly by allowing people to create a plot object that looks like the default plot, and then getting DistributionChart to call MultiDistributionChart with this plot object. This means that it exposes showLegends colorScheme and opacity props (the difference between the two charts).

Soon (after the typescript refactor) I'll add back Plot.dists and we can be more careful about the fields and syntax on this record. However, if you must, we can rehide these settings.

@Hazelfire
Copy link
Collaborator Author

I've just realised this has a regression for summary tables! I'll fix that now.

@Hazelfire
Copy link
Collaborator Author

Hazelfire commented Nov 16, 2022

This also adds multiple distribution summary tables! See below:

{distributions: [{name: "GiveDirectly", distribution: 1 to 2}, {name: "AMF", distribution: 5 to 12}]}

image

Copy link
Collaborator

@berekuk berekuk left a comment

Choose a reason for hiding this comment

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

const r = toJsonRecord(record);
console.log(r);
const plotRecord = schema.validateSync(r);
console.log(plotRecord);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug logging.

),
showLegend: yup.boolean().default(true),
colorScheme: yup.string().default("category10"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

How much do we care about backward compatibility on this?
I expect we'll drop Vega at some point in favor of d3 or something else, and supporting all color schemes would be a lot of work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Upd: I wrote this comment before I saw your comment above that this is temporary.

.shape({
name: yup.string().required(),
distribution: yup.mixed().required(),
opacity: yup.number().default(0.3),
Copy link
Collaborator

Choose a reason for hiding this comment

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

(this comment is not very important and can be ignored; I understand that this plots-as-squiggle-records syntax is temporary and we don't expect to announce it too widely)

I think in cases like this we should restrict the API more, unless there's a clear need for flexibility. How about transparent: yup.boolean()? Or maybe we don't have to expose the this field on yup schema at all?

Any extra field like this:

  • gives users more flexibility
  • but make representation changes for us more costly (if we ever decide to render plots differently, but we gave users control over tiny details of Vega, e.g. colors and line thickness and so on, we won't be sure that the new representation is compatible with existing heavily customized plots)
  • sometimes it can put more burden on users to pick the good-looking defaults

Copy link
Collaborator

Choose a reason for hiding this comment

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

Upd: I wrote this comment before I saw your comment above that this is temporary.

@Hazelfire
Copy link
Collaborator Author

The error that shows is now a bit verbose:
image

But otherwise, it's fine. It does incentivise me to fix #530 .

@berekuk
Copy link
Collaborator

berekuk commented Nov 19, 2022

The error that shows is now a bit verbose

I replaced it with XIcon wrapped in tooltip.

Copy link
Collaborator

@berekuk berekuk 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!

@berekuk berekuk merged commit c8f1e17 into develop Nov 19, 2022
@berekuk berekuk deleted the split-distribution-chart branch November 19, 2022 15:03
@joel-becker
Copy link

joel-becker commented Feb 3, 2023

found a weird inconsistency here @Hazelfire @berekuk

neither of the above approaches work in playground. they do work for me in vscode.

on the other hand, the old Plot.dists approach does work in playground, but does not work for me in vscode!

my squiggle vscode version is v0.5.1. i see that theres a more recent version, but (1) it doesn't show up for me inside vscode and (2), more importantly, if playground is more up-to-date than my local version, i should observe the opposite compatibility pattern.

EDIT: i've noticed that the new approaches don't work in vscode for SampleSet distributions. @Hazelfire seems to get around that here by converting his distributions into lists and then applying sampleset... not sure why, nor how to get such a list from my distributions?

@berekuk
Copy link
Collaborator

berekuk commented Feb 6, 2023

@joel-becker Right, we've added Plot.dists back at some point, and removed the raw record syntax. Sorry for the confusion.

With VS Code, the problem is that the release for 0.6.0 is still marked as pre-release, I think because when I did 0.6.0 release, VS Code marketplace was down and wouldn't accept the new version. We'll do 0.6.1 soon-ish, but in the meantime you can switch to pre-release through VS Code interface:

Captura de pantalla 2023-02-06 a la(s) 12 05 32

(0.5.99 extension version matches 0.6.0-alpha.1 squiggle release, which is identical to final 0.6.0 release)

I'm not sure why plotting samplesets won't work, but it seems to work fine now with Plot.dists.

@joel-becker
Copy link

thank you very much @berekuk ! will try this out later. until then could i bug @Hazelfire for how he converts distributions to lists of samples (to be plotted as distributions, as here)? is there some way to export samples -- for use in squiggle, or elsewhere? this feature would be immensely helpful to me. (getting plots i want much easier outside squiggle.)

@Hazelfire
Copy link
Collaborator Author

Currently, if you are in a JS environment, you can look at the resulting distributions and pull those samples out into JS land to plot differently. My solution is hacky, and was built because Squiggle doesn't yet have a standard way of importing/exporting/serialising distributions. I'm not sure what environment you are using, but you may want a VS code action that can export distributions.

Exporting and importing distributions from different projects was a project that I was going to look into, and this is the second reminder I've gotten that I should probably look into it again. Serializing Squiggle objects into a standard format seems like it would be useful in a couple of contexts, such as caching. Could be worth putting on the todo list sooner rather than later.

Feel free to book a call with me if you want to chat about it further.

@joel-becker
Copy link

thanks @Hazelfire :) booking a call in case it will be helpful for (1) me to probe the JS environment thingy and/or (2) you to use me as focus group for feature prioritization. if neither are true, feel free to cancel.

fwiw, i think for private benefits alone i would pay $500 for this rn. more if thinking about it as contribution to public good, or if using employer money. off top of my head, the next highest willingness to pay for a squiggle feature is within-squiggle saving to enable more costly pipelines at <$100.

@berekuk
Copy link
Collaborator

berekuk commented Feb 8, 2023

@joel-becker List.toString(number[]) or toJSON(anySerializableObject) would be pretty easy to implement, would that help?

(List.toString is even a documented, but it doesn't exist)

You can also use inspect; check out this example, you can copy the array from console in dev tools.

@joel-becker
Copy link

@berekuk thank you! anything that lets me save samples in such a way that i could import into R/python would be super helpful.

  1. i'm not sure about List.toString, because i'm not sure how i'd export the string? (maybe you mean something very similar to inspect + dev tools, except strings might be easier to parse than lists?)
  2. toJSON(anySerializableObject) sounds great.
  3. by inspect + dev tools i think you mean: create a list in squiggle, toggle developer tools in vscode, search for the list inside developer tools, copy-paste the html code containing the list into a new file, use an html parser to transform the list into an R/python-interpretable list. is that correct? if so, this is a fantastic short-term hack -- thank you! -- but the other solutions would still be great (in particular, because i'm not sure how i could automate the toggle-search-copypaste part of this pipeline).

@berekuk
Copy link
Collaborator

berekuk commented Feb 8, 2023

i'm not sure about List.toString, because i'm not sure how i'd export the string? (maybe you mean something very similar to inspect + dev tools, except strings might be easier to parse than lists?)

Since inspect requires you to open dev tools, which feels hacky, initially I thought of something like this: normal(0,1) -> SampleSet.fromDist -> SampleSet.toList. But the output format is not right.

So there are some possible improvements:

  • convert to string on Squiggle side, output the string in the playground, and then copy it manually (that's toString idea)
  • also, we could add "export to JSON" button in the playground; I think I'll do that eventually, could be useful in general

by inspect + dev tools i think you mean: create a list in squiggle, toggle developer tools in vscode, search for the list inside developer tools, copy-paste the html code containing the list into a new file, use an html parser to transform the list into an R/python-interpretable list.

You won't need to parse it. I meant the inspect() function in Squiggle (check the list in my previous comment). It outputs the data to the devtools console, and you can copy it from there in the standard [1,2,3] format.

The API is:

  • inspect([1,2,3]) outputs [1,2,3]
  • inspect([1,2,3], "foo") outputs foo: [1,2,3], so that you could identify it more easily

@joel-becker
Copy link

thank you very much again @berekuk -- this is super helpful!

i'm not understanding this piece:

It outputs the data to the devtools console, and you can copy it from there in the standard [1,2,3] format.

I start with the script + preview.

Screen Shot 2023-02-08 at 6 55 21 PM

I open up devtools, go to console tab.

Screen Shot 2023-02-08 at 6 58 04 PM

there are far fewer than 1k samples listed and, besides, I'm not sure how to "copy it from there in the standard [1,2,3] format." do you have a sense for where i might be going wrong? (thank you for your patience with this devtools novice!)

@berekuk
Copy link
Collaborator

berekuk commented Feb 9, 2023

In your example, d is a single-argument function. So it doesn't get evaluated when the code is executed, and those logs are from rendering the chart (unrelated to the sampleset at all!).

In my version, that anonymous {|list| inspect(list, "x")} function was following the -> arrow:

normal(5,2) ->
  SampleSet.fromDist ->
  SampleSet.toList ->
  {|list|inspect(list, "x")}

-> semantics is "pass the argument on the left to the function on the right". So that snippet of code is the same as:

f = {|list|inspect(list, "x")}
f(SampleSet.toList(SampleSet.fromDist(normal(5,2))))

Or, simplifying further:

list = SampleSet.toList(SampleSet.fromDist(normal(5,2)))
list = inspect(list)

(the "x" part is optional)

@joel-becker
Copy link

aha! got it! thank you once again vyacheslav!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants