feat: add VegaLite.Data.polar_plot #69

polvalente · 2023-08-19T09:42:09Z

Adds the ability for plotting polar data

lib/vega_lite/data.ex

cristineguadelupe · 2023-08-19T10:13:45Z

Hey @polvalente! I like the idea of supporting it, but I agree with @josevalim. How about a new module? 🤔

polvalente · 2023-08-19T10:15:05Z

Hey @polvalente! I like the idea of supporting it, but I agree with @josevalim. How about a new module? 🤔

I'm not set on keeping this in VegaLite.Data. It just seemed like the right place to do it.

lib/vega_lite/data.ex

josevalim · 2023-08-19T10:17:13Z

lib/vega_lite/data.ex

+    radius_marks_vl ++ radius_ruler_vl
+  end
+
+  defp polar_infer_radius_marks(data) do


We don't do data processing. And we can't assume data is an enum nor fields in the data. We only assume data implements Table.Reader.

This is a deal breaker in terms of usability, but again, can be extracted out.

The rest of the preprocessing is done through VegaLite calculations, so it's fine

Yeah, it is up to you. I am mostly dropping comments in favor of consistency with the remaining APIs in this module, which I would like to strictly adhere to for now.

It's not clear to me.

If we:

remove the max radius inference

refactor the data keys so that we don't assume the keys in data

only accept a single data collection instead of a list of those

Is the feature good to go? Or would it still be in the "can be denied" realm?

Then I think we can accept it, if you still think it is useful. But I can't promise other blockers won't be found as you refactor. I can't promise to merge something based on code i can't see.

cristineguadelupe · 2023-08-19T10:18:13Z

Hey @polvalente! I like the idea of supporting it, but I agree with @josevalim. How about a new module? 🤔

I'm not set on keeping this in VegaLite.Data. It just seemed like the right place to do it.

I'd say that VegaLite.Data should be reserved for data that implements Table.Reader.
A new module seems ok to me, I don't think VegaLite will ever support it directly, they're restricted to operating on top of grammars rather than abstractions.

josevalim · 2023-08-19T10:26:37Z

lib/vega_lite/data.ex

+    opts =
+      case opts[:radius_marks] do
+        nil ->
+          radius_marks = polar_infer_radius_marks(data)


One alternative is to infer the min and max using VegaLite, if at all possible.

I wouldn't be because we plot separate layers for each radius mark

Keep in mind though you can keep the data on the root, outside of the layers, then all layers will have access to it. You can see how other functions do it. My suggestion would be to trim this down into the smallest possible function, that follows the other patterns in this module, and then you can add all of the conveniences later in a pull request.

I was wrong about them being separate layers, so maybe we can do some kind of vega-lite calculation for this. Though I don't see this inference missing as a blocker for the feature

Keeping the data in the root hurts composability in a sense because I have different r/theta definitions depending on whether we're dealing with the angle marks, the radius marks or the data layers

polvalente · 2023-08-19T12:56:05Z

lib/vega_lite/data.ex

+        legend_key: legend_key
+      )
+  """
+  def polar_grid(vl \\ Vl.new(), opts \\ []) do


@josevalim @cristineguadelupe I was able to split polar_grid and polar_plot (docs still pending, I'll leave this for later).

The idea is that polar_grid will just generate an empty polar grid, while polar_plot will convert polar coordinates data into a cartesian plot.

Both can be combined to plot multiple datasets onto a given grid.

I also implemented the append_layers defp which we might want to extract.

Forgot to show just vl_grid:

This looks pretty good. I think we are on a path to merging this 🎉

lib/vega_lite/data.ex

josevalim · 2023-08-19T15:53:26Z

lib/vega_lite/data.ex

+    vl_polar_config =
+      case vl.spec do
+        %{"_vl_polar_config" => conf} -> Enum.to_list(conf)
+        _ -> [radius_marks: [1, 2, 3, 4, 5], angle_offset: 0, direction: :counter_clockwise]


I think we should raise if there is no config.

If there's no config, we can't do the standalone plot (the one where we get the cartesian-converted plot for polar data). Maybe we can require that either the config is present or those three options are present.
If both are present, we could make it so that the config takes precedence.

If there's no config, we can't do the standalone plot (the one where we get the cartesian-converted plot for polar data).

That's fine by me. Let's raise for now. It is always easier to add features later.

I managed to use the fields along with a check for the presence of the :radius_marks config to allow for the plot to still be used in a standalone fashion.

I'll commit it like this and we can remove the code if you feel it's bloat

josevalim · 2023-08-19T15:53:35Z

lib/vega_lite/data.ex

+  end
+
+  def polar_plot(vl \\ Vl.new(), data, mark, fields, opts \\ []) do
+    opts = Keyword.validate!(opts, scheme: "turbo", hide_axes: false)


Should we set the scheme? I don't think so.

I changed it to be optional

It is not about being optional. I don't think we should change the scheme unless the user asks us to. We don't do that for any other the other plots.

I've added the color field as an optional meta-field for this configuration. It also is now what affects the legend grouping.

lib/vega_lite/data.ex

josevalim · 2023-08-20T20:39:22Z

lib/vega_lite/data.ex

+      case fields[:r] do
+        opts when is_list(opts) -> opts[:field]
+        field when is_binary(field) -> field
+      end


We have a function for normalizing fields. Can you please use it instead? We also have functions to extract information from data and subfields. Please use chart as the foundation for this function and then build on top!

I can see how to use that, but I don't see how columns_for applies to this plot

lib/vega_lite/data.ex

josevalim · 2023-08-20T20:46:17Z

lib/vega_lite/data.ex

+    * `:x` - options for the x-axis
+    * `:y` - options for the y-axis


Suggested change

* `:x` - options for the x-axis

* `:y` - options for the y-axis

Let's remove options for :x and :y for now. The most we can simplify this for now, the better.

This will make it so we can't have the standalone cartesian-converted plot. This is what you want?

Yes, we can wait for a use case.

Co-authored-by: José Valim <jose.valim@gmail.com>

…ite into pv-feat/add-polar-plot

josevalim

LGTM! We now need tests and i think we will be ready to go!

josevalim · 2023-08-30T07:53:02Z

We are deprecating the specialized plots in VegaLite.Data. :) Thank you @polvalente and sorry for all the back and forth on this.

feat: add polar plot

7bd1f69

josevalim reviewed Aug 19, 2023

View reviewed changes

lib/vega_lite/data.ex Outdated Show resolved Hide resolved

josevalim reviewed Aug 19, 2023

View reviewed changes

lib/vega_lite/data.ex Outdated Show resolved Hide resolved

josevalim reviewed Aug 19, 2023

View reviewed changes

polvalente added 2 commits August 19, 2023 09:26

wip: some refactors

4b72275

feat: hide meta-config inside VL spec

8378834

polvalente commented Aug 19, 2023

View reviewed changes

josevalim reviewed Aug 19, 2023

View reviewed changes

lib/vega_lite/data.ex Outdated Show resolved Hide resolved

josevalim reviewed Aug 19, 2023

View reviewed changes

lib/vega_lite/data.ex Outdated Show resolved Hide resolved

josevalim reviewed Aug 19, 2023

View reviewed changes

lib/vega_lite/data.ex Outdated Show resolved Hide resolved

polvalente added 2 commits August 20, 2023 17:08

refactor: changes due to code review

1bd3038

refactor: rename legend->color field

059aa1a

josevalim reviewed Aug 20, 2023

View reviewed changes

lib/vega_lite/data.ex Outdated Show resolved Hide resolved

josevalim reviewed Aug 20, 2023

View reviewed changes

lib/vega_lite/data.ex Show resolved Hide resolved

josevalim reviewed Aug 20, 2023

View reviewed changes

lib/vega_lite/data.ex Show resolved Hide resolved

josevalim reviewed Aug 20, 2023

View reviewed changes

polvalente and others added 5 commits August 20, 2023 17:47

Update lib/vega_lite/data.ex

b2af6e1

Co-authored-by: José Valim <jose.valim@gmail.com>

refactor: use normalize_fields

b8c947a

Merge branch 'pv-feat/add-polar-plot' of github.com:polvalente/vega_l…

c2aafe6

…ite into pv-feat/add-polar-plot

chore: move defps around

25377ff

refactor: remove x-y config support

423b40a

josevalim reviewed Aug 21, 2023

View reviewed changes

test: add test for polar plot

e19c8ea

test: add test for argument error

3b9c389

polvalente requested a review from josevalim August 29, 2023 14:54

polvalente mentioned this pull request Aug 29, 2023

Polar Plot pnezis/tucan#7

Open

josevalim closed this Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add VegaLite.Data.polar_plot #69

feat: add VegaLite.Data.polar_plot #69

polvalente commented Aug 19, 2023

cristineguadelupe commented Aug 19, 2023

polvalente commented Aug 19, 2023

josevalim Aug 19, 2023

polvalente Aug 19, 2023

polvalente Aug 19, 2023

josevalim Aug 19, 2023

polvalente Aug 19, 2023

josevalim Aug 19, 2023

cristineguadelupe commented Aug 19, 2023

josevalim Aug 19, 2023

polvalente Aug 19, 2023

josevalim Aug 19, 2023

polvalente Aug 19, 2023

polvalente Aug 19, 2023

polvalente Aug 19, 2023

polvalente Aug 19, 2023

josevalim Aug 19, 2023

josevalim Aug 19, 2023

polvalente Aug 20, 2023

josevalim Aug 20, 2023

polvalente Aug 20, 2023

josevalim Aug 19, 2023

polvalente Aug 20, 2023

josevalim Aug 20, 2023

polvalente Aug 20, 2023

josevalim Aug 20, 2023

polvalente Aug 20, 2023

josevalim Aug 20, 2023

polvalente Aug 20, 2023

josevalim Aug 20, 2023

polvalente Aug 20, 2023

josevalim left a comment

josevalim commented Aug 30, 2023

		* `:x` - options for the x-axis
		* `:y` - options for the y-axis

feat: add VegaLite.Data.polar_plot #69

feat: add VegaLite.Data.polar_plot #69

Conversation

polvalente commented Aug 19, 2023

cristineguadelupe commented Aug 19, 2023

polvalente commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristineguadelupe commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

josevalim commented Aug 30, 2023