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

Sunburst value accessor issue #1444

Closed
wants to merge 3 commits into from
Closed

Sunburst value accessor issue #1444

wants to merge 3 commits into from

Conversation

kum-deepak
Copy link
Collaborator

@kum-deepak kum-deepak commented May 21, 2018

Attempt to fix #1440.

@kum-deepak
Copy link
Collaborator Author

kum-deepak commented May 22, 2018

I have cleaned up the example.

@gordonwoodhull, the current example uses Reductio, which may be good. However it is possible to demonstrate the concept using multiple group sums similar to Stocks example. Will wait for your advice.

If we continue with Reductio, currently reductio is loaded directly from CDN, it might be better to use npm to install and Grunt to copy it to our js folder, like other similar cases. Again, will wait for your advice.

@gordonwoodhull
Copy link
Contributor

Tricky... I forgot that sunburst was also capped, so now we have a double-wrapped accessor. I don't see why it shouldn't work though.

I think if the example used a keyAccessor, we would find that we needed an extendedKeyAccessor as well as an extendedValueAccessor. Your call whether we add that now or wait for someone to file another issue.

I think it's nice to have an example that uses reductio, especially since reductio can help a lot with complicated reductions like this. I agree we should add it as a devDependency rather than loading it from the CDN, even though it's probably safe.

@gordonwoodhull
Copy link
Contributor

I'd prefer if the data had a descriptive filename. @mukherjeea, are you okay with us using this data in an example, and if so, what does it mean?

@mukherjeea
Copy link

@gordonwoodhull the data I've used in the fiddle is from production. I will need to mask couple of attributes before it can be used as an example. Give me some time. I will update you guys.

@kum-deepak
Copy link
Collaborator Author

@gordonwoodhull, thanks for your responses, it all makes sense.

It does intuitively sound to me that keyAccessor would have same issue, however, I am unable to come up with an example. So, while I can make the change, do not want to do it without producing the problem first 😄.

There is one more fundamental thing I have breaking my head on. Does using Capping actually help (or even work for) Sunburst charts. I guess it is there because the original code was picked from Pie chart. Sunburst charts create a hierarchy. If someone uses Capping - the hierarchy will be incorrect (as may nodes would get collapsed into 'Others' bucket before the computation). I am unable to think about correct behavior of Capping in this context. At this stage none of the examples use Capping. What is your opinion - should we drop Capping from Sunburst?

@gordonwoodhull
Copy link
Contributor

@mukherjeea, any luck producing a data set we could use for the test? Unfortunately we are testing based on the example you gave in #1440, so I can't currently merge this fix without merging that data.

@mukherjeea
Copy link

mukherjeea commented Jul 11, 2018

@gordonwoodhull wasn't able to revisit this. This fiddle has a usable dataset. I've also updated the code that follows. sorry about the delay.

https://jsfiddle.net/brL1x26g/21/

@mukherjeea
Copy link

dc_example_masked.csv.zip

the dataset for completeness.

@gordonwoodhull
Copy link
Contributor

Thanks @kum-deepak, @mukherjeea!

I merged the new data into the example and squashed, so the unmasked data is gone from the main branch (it will disappear entirely once @kum-deepak deletes this branch).

Thanks for providing clear field names on the new dataset - with that info I was able to call the dataset ordered_returned.csv - I like that much better than naming it after the feature it's demonstrating.

We can fix the keyAccessor if it turns out to be a problem in the future.

Merged for 3.0.6

@kum-deepak kum-deepak deleted the sunburst-value-accessor-issue branch July 12, 2018 02:46
@kum-deepak
Copy link
Collaborator Author

Deleted my branch. Thanks!

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.

sunburst chart doesn't render when key reduced to multiple values
3 participants