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

[WIP] Pie chart #204

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

[WIP] Pie chart #204

wants to merge 7 commits into from

Conversation

tusharpm
Copy link

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.

@9prady9
Copy link
Member

9prady9 commented Jul 22, 2019

@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
Copy link
Member

9prady9 commented Jul 22, 2019

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

Copy link
Member

@9prady9 9prady9 left a comment

Choose a reason for hiding this comment

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

Code's good. I will try to make a PR that lets user or at least internal API to turn off grid and traditional 2d plot things that aren't helpful with Pies.

Have you thought about how to put some text related to pie %'s ?

src/backend/opengl/pie_impl.hpp Outdated Show resolved Hide resolved
include/fg/pie.h Outdated Show resolved Hide resolved
src/backend/glsl_shaders/pie_gs.glsl Show resolved Hide resolved
@tusharpm
Copy link
Author

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.

@9prady9
Copy link
Member

9prady9 commented Jul 24, 2019

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
Copy link
Member

9prady9 commented Jul 29, 2019

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
Copy link
Member

9prady9 commented Jul 29, 2019

I have created the #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
Copy link
Member

9prady9 commented Aug 12, 2019

@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 ?

@tusharpm
Copy link
Author

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?

@9prady9
Copy link
Member

9prady9 commented Aug 21, 2019

@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
Copy link
Member

9prady9 commented Dec 24, 2019

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

@9prady9
Copy link
Member

9prady9 commented Mar 25, 2024

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

@9prady9 9prady9 marked this pull request as draft March 25, 2024 04:43
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.

2 participants