-
Notifications
You must be signed in to change notification settings - Fork 88
Conversation
Hi Parker, I was wondering if you knew approximately what the timeframe for this feature being merged/released might be? We're looking at using it in cbioportal.org but were wondering if it's something we should count on being out soon. Thanks for any response and for your work on this very useful project! -Adam |
@adamabeshouse Thanks for checking in. I know @boygirl is planning to spend some time working on this soon, but is headed on vacation for a few weeks soon. I may get some time to work on it over weekends, but I wouldn't count on it being around imminently – I'm not currently on our OSS team so I have to do these things after hours when I can. |
Thanks, @parkerziegler! |
@adamabeshouse this one is next up on my list. I'm on vacation until March 12, but I expect to finish this feature by the end of March at the latest. |
Thanks for the information @boygirl ! Enjoy your vacation. - Adam |
…leLabs/victory-chart into feature/victory-boxplot
@boygirl Sorry this is coming in late – I had an epiphany as I went to start this PR on Friday and wanted to get in some changes.
Purpose
This PR contains initial work for a
VictoryBoxPlot
component to be introduced intovictory-chart
. This component uses primitives created invictory-core
on the branch of the same name (feature/victory-boxplot
). To get it running, you'll need to follow thelank
workflow using the branch of the same name fromvictory-core
. This component is functional but by no means complete. More work needs to be done to ensure it conforms to standards for other components invictory-chart
. Tests also need to be written for this component. This branch should not be merged in its current state.Component Structure
To see how this component is being used, check out
victory-boxplot-demo.js
.VictoryBoxPlot
This PR introduces two new files to the library –
victory-boxplot.js
andhelper-methods.js
undersrc/components/victory-boxplot
.Working with Data
The
VictoryBoxPlot
component accepts data in three formats at this time.data={[{ x: 1, y: 3 }, { x: 1, y: 5 }, { x: 1, y: 10 }]}
etc. When received in this form, we make certain assumptions about the data:x
ory
values. For example, we would not handle data in this format if it was passed like sodata={[{ x: 1, y: 3 }, { x: 2, y: 5 }, { x: 3, y: 10 }]}
. Future work could handle grouping entries by their independent variable value automatically.x
ory
), the component requires ahorizontal
prop of typeboolean
. When set to true, the component will expect repeaty
values and render the box plot horizontally (withmin
on the left andmax
on the right). The default value forhorizontal
isfalse
.data={[{ x: 1, y: [2, 3, 5] }, { x: 1, y: [3, 5, 7, 8] }, { x: 1, y: [5, 10, 25, 32 }]}
.datum
as corresponding to a separate box plot, i.e. the above example would render three separate box plots.data={[{ min: 1, max: 10, q1: 2.5, q3: 6, med: 5 }]}
. If an array of objects of the above form is passed, a separate box plot will be rendered for each.Processing Data
In order to accept the data in all of the above forms, we run the data through a set of methods to determine how to process it. These are all available in
helper-methods.js
and should be checked for efficiency and efficacy. The current checks we have in place:data
is passed in the pre-processed format –this.checkProcessedData
.this.checkHasQuartileAttributes
to ensure that all necessary quartile attributes (min
,max
,q1
,q3
, andmed
) are present on thedatum
.this.checkHasDistinctIndependentVariable
, which ensures that eachdatum
has a uniquex
ory
attribute (whichever is the independent variable in this case, which is determined using thehorizontal
prop).data
contains a dependent variable passed as an array (the second data case mentioned above) –this.checkDependentVariableAsArray
.d3
'squartile
methods work properly.datum
–this.getSummaryStatistics
.Rendering Data
Rendering
VictoryBoxPlot
is complicated by the fact that there is no singledataComponent
for each part of the box plot. Instead, we have the following mapping:This means that the typical
this.renderData
method inadd-events
will not work neatly here. Instead, methods are present directly onVictoryBoxPlot
that read from the data structure passed and use the data structure keys toclone
the correct component. Similarly, in order to clone the correctprops
for each component (andlabels
andlabelProps
) we use the data structure keys. So, each box plot arrives to ourrender
method in the following format:We then call
this.getBoxPlotComponent
andthis.getLabelComponent
to obtain the proper component and props for each key. This could almost certainly be done in a more elegant way and I would love to see some improvement in this area.Getting the Domain for All Box Plots
There is a custom method in
helper-methods.js
for obtaining the properx
andy
domains to contain all box plot datasets passed intodata
. It takes advantage of the fact that we already have themin
andmax
values for each dataset, and uses these to determine the domain of the dependent axis across all datasets. It obtains the domain of the independent axis as usual.Suggested Enhancements and Future Work
This component still needs considerable work to behave properly and conform to the standards of other
victory-chart
components. Some things to direct attention to:data
prop.Helpers
to allow props likeboxWidth
andwhiskerStyle
to accept props as eitherstring
s ornumber
s.events
andtransitions
get properly attached to the component. Most of this should be taken care of byadd-events
but I have not verified.victory-native
.demo
examples and tests (currently there are none).labels
and use ofLabelHelpers.getText
- at the moment we are just rendering the computed values formin
,max
,q1
,q3
, andmed
as place holders.boxWidth
,whiskerStyle
, andlabelOrientation
– are these doing what we expect and are there any gotchas?accessor
functions for potential improvements.horizontal
prop. Currently, we passhorizontal
into nearly every calculation because it dramatically affects positioning. However, there are ternary expressions all over the place that are rapidly inflating the complexity calculations of some methods – and ESLint is not happy about it.PropTypes
anddefaultProps
.displayScatter
prop that, when set totrue
, renders a scatter plot of the raw data on top of the boxplot. Additionally, we may want to add adisplayJitter
prop that does the same thing but creates a jitter plot by moving the points slightly along the independent axis.