Skip to content

Fix aggregate chart xaxis labels#1922

Merged
aestoltm merged 6 commits intoubccr:xdmod11.0from
aestoltm:fix_aggregate_xaxis_labels
Jan 2, 2025
Merged

Fix aggregate chart xaxis labels#1922
aestoltm merged 6 commits intoubccr:xdmod11.0from
aestoltm:fix_aggregate_xaxis_labels

Conversation

@aestoltm
Copy link
Copy Markdown
Contributor

@aestoltm aestoltm commented Oct 7, 2024

Description

Fix aggregate chart xaxis labels when multiple statistics are plotted for a single yaxis object. The issue was that with multiple aggregate data series, the first trace determines the x axis labels for the chart. If the following data series objects contain xvalues that are not within the first trace then those values are ignored (null'ed).

The following snippet shows how the xvalues for each yaxis object starts as an empty array of null values. Values are only updated if a match exists within the xAxis object. Looking at getYAxis() in ComplexDataset.php

$xValues = array_fill(
                    0,
                    $this->_xAxisDataObject->getCount(),
                    null
                );
foreach ($subXAxisObject->getValues() as $xIndex => $xValue) {
                    $found = array_search(
                        $xValue,
                        $this->_xAxisDataObject->getValues(),
                        true
                    );

                    if ($found !== false) {
                        $newValues[$found] = $yAxisDataObject->getValue($xIndex);
                        $xIds[$found]      = $subXAxisObject->getId($xIndex);
                        $xValues[$found]   = $subXAxisObject->getValue($xIndex);

                        if (
                            $dataDescripterAndDataset->data_description
                                                     ->std_err
                        ) {
                            $newErrors[$found]
                                = $yAxisDataObject->getError($xIndex);
                        }
                    }
                } // foreach ($subXAxisObject->getValues() ...

The reason this was not an issue before is that Highcharts set categorical axis labels through the xAxis object, whereas, Plotly JS has its category labels set through the data object itself. This should have been changed from the start of the transistion to Plotly to use the xAxis object to receive the xAxis values.

Before:
aggregate_labels_bug

After:
chart_after

Motivation and Context

Aggregate charts that contain multiple statistics at once will have blank labels if the label does not exist in every trace.

Tests performed

Tested on my dev port

Checklist:

  • The pull request description is suitable for a Changelog entry
  • The milestone is set correctly on the pull request
  • The appropriate labels have been added to the pull request

…y from xAxis object instead of using yAxisObject->getXValue()
@aestoltm aestoltm added the bug Bugfixes label Oct 7, 2024
@aestoltm aestoltm added this to the 11.0.1 milestone Oct 7, 2024
@aestoltm aestoltm self-assigned this Oct 7, 2024
@jpwhite4
Copy link
Copy Markdown
Member

jpwhite4 commented Oct 7, 2024

There seem to be more bars in the second plot than the first one. How come?

@aestoltm
Copy link
Copy Markdown
Contributor Author

aestoltm commented Oct 7, 2024

There seem to be more bars in the second plot than the first one. How come?

It looks like there are more bars on xdmod-dev even without the proposed changes.

@aestoltm aestoltm changed the base branch from main to xdmod11.0 November 22, 2024 18:58
@aestoltm aestoltm merged commit e6f770d into ubccr:xdmod11.0 Jan 2, 2025
@aestoltm aestoltm deleted the fix_aggregate_xaxis_labels branch January 2, 2025 21:32
aaronweeden pushed a commit to aaronweeden/xdmod that referenced this pull request Feb 10, 2025
Fix aggregate chart xaxis labels by correctly using xAxis values array from xAxis object instead of using yAxisObject->getXValue()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bugfixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants