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

#2175 & #3723 - Multicategory Multilevel 2+ & Sorting Multicategory #6327

Open
wants to merge 62 commits into
base: master
Choose a base branch
from

Conversation

richardnm-2
Copy link

#2175 requests for a multicategory multilevel (2+). This is great for data visualization on, as mentioned, "Variability Charts".

In order to expand the capabilities of multicategory axis type, the list structure previously implemented has been changed at set_convert.js ax.setupMultiCategory(), adding a new file to src/lib -> sort_traces.js. Working with the traces as a list of objects instead, to try to provide the same data structure to setCategoryIndex() seemed easier. Sorting is performed reversed, in order to achieve a stable sorting. I don't know about the implementation, but the result is the same as if sorted backwards in pandas (Python), or an Excel table.

The capabilities upgrade was then achieved, and the other changes were to make tha axis draw the new data shape, as pushing multiple functions to seq, in a loop, to account for every level. Also, category indexes had to be updated to comply with the new data shape.

The categorical data on the axis was drawn accordingly, but the sorting issue, #3723 showed that this was a known problem. Testing showed that providing the data already sorted to the newPlot function outputed the desired result. So, as the trace levels were already beein looped inside ax.setupMultiCategory(), and the object list was easyly sortable, before transforming it to a matrix again, it made sense to also update gd._fullData with the sorted data. The place to do so may not be the best and some thought can be given to that.

@alexcjohnson
Copy link
Collaborator

@richardnm-2 thanks so much for opening this PR! Just a couple of structural comments while I'm reviewing the rest of the code:

  • Per the contributing guide please use node 16.x / npm 7.x - or at least don't commit the package-lock.json generated by an earlier version.
  • This PR shouldn't touch anything in the devtools/test_dashboard dir. Instead let's add one or more new figures in test/image/mocks, then you can load these in the regular dashboard, but they also become part of the test suite. If you can't get the baseline generation part to work we can help with that.

return newMatrix;
}

exports.transpose = transpose;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't fully worked through what you're doing with this, but note there's also a matrix module with its own transposition function, transposeRagged. If that routine doesn't do what you need, let's move this transpose into the matrix module and call it perhaps transposeSquare. Same goes for other functions here that aren't specific to traces per se.

if(d) return d;
if(axLabels.length) {
ax.levelNr = axLabels[0].length;
ax.levels = axLabels[0].map(function(_, idx) {return idx;});
Copy link
Collaborator

Choose a reason for hiding this comment

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

ax is part of gd._fullLayout and it's therefore a kind of "cleaned" and "completed" version of gd.layout, the layout portion of the figure. With that in mind, it's OK to add things to ax that don't exist in layout.xaxis etc, but only if you prefix them with an underscore.

var axLabels = [];
var fullObjectList = [];
var cols = [];
// Don't think that the trace should be drawn at all if the lengths don't match. Removing the arrays length check. It is better to fail loudly than silently.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For better or worse that's not the approach plotly.js takes. Partly because it gets used in manual data entry applications and other cases where a plot gets drawn before the data is complete, we almost always do our best to NOT fail, and just interpret as much of the data as we can and draw that.


if(trace.type === 'ohlc' | trace.type === 'candlestick') {
var sortedValsTransform = sortLib.transpose(axVals);
gd._fullData[i].open = sortedValsTransform[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to my previous comment about adding things to ax, we can't be overwriting contents of gd._fullData in a way that conflicts with the contents of gd.data, but if necessary we can add underscore-prefixed attributes.

Also: this section looks a bit too tightly coupled with concerns that belong to individual trace types. If we need to have logic here that's specific to one or two trace types, that functionality should go in the trace module and get called from there, something like:

var traceSetup = gd._fullData[i]._module.setupMultiCategory;
if(traceSetup) {
    traceSetup(gd._fullData[i], sortedValsTransform);
}

Then candlestick and ohlc would opt into this behavior by defining a setupMultiCategory function in their index files

@AndreasUntch
Copy link

@richardnm-2 Hello, great to see that someone is working on this and tries to provide a solution :) It is quite a pain with multicategory axis right now. Sorting breaks whenever an item is missing in one stack for example... Hope there will be a fix soon. fyi: We are using plotly in R

@zupdaz
Copy link

zupdaz commented Jun 28, 2023

@richardnm-2 It's fantastic to see your efforts to improve the current handling of multicategory axes. Currently, when a category lacks data points, it can significantly disrupt the visualization integrity. Also, having more than 2 levels is a must for most of modern data analysis. Is this feature still being worked on at the moment?

@PhilippaTreacy
Copy link

Is this close to merging? Would be a great feature to have.

@DianYing123
Copy link

DianYing123 commented Jan 24, 2024

@richardnm-2 May I inquire about the approach you used to resolve this issue in plotly.py?

@alon-aviv-ni
Copy link

Hi, is this PR still in progress? Looking forward for this functionality to be introduced in the library.

@wowoxiapqi
Copy link

I really hope this feature will be ok

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.

None yet

8 participants