forge icon indicating copy to clipboard operation
forge copied to clipboard

[WIP] Pie chart

Open tusharpm opened this issue 4 years ago • 11 comments

What kind of change does this PR introduce? Feature.

What is the current behavior? #68 lists pie chart as an option for statistical plots.

What is the new behavior? Add Pie chart.

Does this PR ensure that key events handling across different window toolkits is identical if it has changes related to window toolkits? N/A.

Does this PR introduce a breaking change? No. Pie API is purely additive.

Please check if the PR fulfills these requirements

  • [ ] The commit message following the guidelines
  • [ ] Examples for all compute backends, CUDA & OpenCL, have been added (for new examples)
  • [ ] Docs have been added / updated (for bug fixes / features)
  • [ ] Change compiles and executes on Linux, OSX and Windows (for window toolkit changes as CI builds don't check for this yet).

Other information: WIP: The branch includes a CPU example - need to write the same for CUDA/OpenCL. Need to find a formal way to disable grid and ticks for the chart object. The API has the doc comments (similar to histogram). Need to update README.md to reflect Pie chart support.

tusharpm avatar Jul 22 '19 06:07 tusharpm

@tusharpm Looks good, yeah, right now there is no way to disable rendering of ticks and grid lines. But I can add those, shouldn't be hard - just want to make sure it is done the right way.

9prady9 avatar Jul 22 '19 06:07 9prady9

I will comment on changes once I look at the shader etc.

9prady9 avatar Jul 22 '19 06:07 9prady9

For the pie %, I'm not sure about a GLSL way of rendering text. (I'll try looking that up). Alternatively, it might need copying the vertex buffer contents locally to process in pie_impl.cpp.

tusharpm avatar Jul 24 '19 16:07 tusharpm

perhaps something like examples/cpu/plotting.cpp does - put legend for each sector in respective color. I will check what are our options there. If you have ideas, please do share.

9prady9 avatar Jul 24 '19 16:07 9prady9

Percentage That is what I have in mind at the moment, anything more like the following Percentage_Ideally

would take more time but makes %'s and labels conspicuous. I think there is disadvantage to the second approach in some cases where larger number of labels and screen real estate becomes cluttered. In this case, the first approach also has an issue of how many labels we can fit in vertical direction while having a legible font size.

9prady9 avatar Jul 29 '19 05:07 9prady9

I have created the https://github.com/arrayfire/forge/pull/205 with necessary changes

You can apply the following diff on your PR to rendering pie without axes.

diff --git a/src/api/c/chart.cpp b/src/api/c/chart.cpp
index e63af51..d21ca75 100644
--- a/src/api/c/chart.cpp
+++ b/src/api/c/chart.cpp
@@ -180,6 +180,7 @@ fg_err fg_append_pie_to_chart(fg_chart pChart, fg_pie pPie)
         ARG_ASSERT(1, (pPie!=0));
 
         getChart(pChart)->addRenderable(getPie(pPie)->impl());
+        getChart(pChart)->setAxesVisibility(false);
     }
     CATCHALL
 
@@ -279,6 +280,7 @@ fg_err fg_add_pie_to_chart(fg_pie* pPie, fg_chart pChart,
 
         common::Pie* pie = new common::Pie(pNSectors, (forge::dtype)pType);
         chrt->addRenderable(pie->impl());
+        chrt->setAxesVisibility(false);
         *pPie = getHandle(pie);
     }
     CATCHALL

9prady9 avatar Jul 29 '19 07:07 9prady9

@tusharpm Great, I see that you have disabled axes rendering now. Sorry, I didn't get notification for that push, so I was under the impression you are still working on it.

What are your thoughts on legends ?

9prady9 avatar Aug 12 '19 08:08 9prady9

I had actually missed the notification for the PR you merged earlier. I'm sorry for the confusion.

I haven't looked into legends, yet. The first image from your comment seems closer to the legends used in plotting.cpp. Calculating and rendering the list elements would require reading data from the vertex and color buffers. Can you share your opinion/suggestions for that approach?

tusharpm avatar Aug 12 '19 17:08 tusharpm

@tusharpm Sorry about the delay, I will think about possible ideas, but as of now, I can only think of it to get the lists of texts(legends) and percentages from user.

The legends from plotting are slightly different because each legend is set individually by each item of 2d chat. However, in the case of pie, we need to push all these into a single item, thus would involve a vector of strings or something like that.

Percentages will be a tricky one for sure. Let me take a look this PR changes once again. I will update here if I have something new.

You can also ping me on slack - https://arrayfire-org.slack.com/messages/C6QTLM3M0 for a real time discussion.

Thank you.

9prady9 avatar Aug 21 '19 09:08 9prady9

I have rebased it from latest master and added remove API for pie renderables too.

9prady9 avatar Dec 24 '19 15:12 9prady9

I will work on this some time in 2024 to revive this feature. So will make this into draft up until the point.

9prady9 avatar Mar 25 '24 04:03 9prady9