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.

Sign up for GitHub

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

Fix for charts with side-by-side grouping (Bar/Area plots) #1883

Open
wants to merge 33 commits into
base: xdmod11.0
Choose a base branch
from
Open

Fix for charts with side-by-side grouping (Bar/Area plots) #1883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
7662078
Fix offset group for chart with data from two different realms
aestoltm Jul 15, 2024
02eb17b
Exclude pie chart fix from this pr
aestoltm Jul 15, 2024
024d917
Fix extra space
aestoltm Jul 15, 2024
9c3d00d
Fix trend line legend ordering
aestoltm Jul 15, 2024
e599bdf
Adjustments to trendline legend position logic
aestoltm Jul 15, 2024
3b8837a
Fix legend rankings
aestoltm Jul 15, 2024
fc0a4f7
Account for the area plot workaround
aestoltm Jul 15, 2024
6b519ea
Fix whitespace error
aestoltm Jul 15, 2024
9db6bb1
Update image hashes
aestoltm Jul 16, 2024
246d023
Fixed group stacking bug when mixing multiple data sources from the s…
aestoltm Jul 17, 2024
b26dc67
update branch with xdmod11.0
aestoltm Oct 2, 2024
41bddf2
Update regression tests after merge conflict
aestoltm Oct 3, 2024
2825714
Merge branch 'main' into bar_chart_fix
aestoltm Nov 8, 2024
f75a9ae
Update legend rank starting value to make rank calculating easier
aestoltm Nov 12, 2024
5732c8d
Update rank calculation to simply be division by 2. Also fix syntax e…
aestoltm Nov 12, 2024
9ac5902
Use metadata flag to determine if primary or supplemental trace inste…
aestoltm Nov 12, 2024
70d36fd
Refactor metadata to reduce code duplication
aestoltm Nov 12, 2024
c32e6c3
Linter fix
aestoltm Nov 12, 2024
f8cc1e2
Added back check for if a series is a trendline supplemental series
aestoltm Nov 12, 2024
d840860
Merge branch 'xdmod11.0' into bar_chart_fix
jpwhite4 Nov 22, 2024
c209f35
Add CHANGELOG/release notes about PR
aestoltm Nov 25, 2024
f8feb74
Merge branch 'xdmod11.0' into bar_chart_fix
aestoltm Nov 27, 2024
7c249e9
Linter fix
aestoltm Dec 3, 2024
27e99e4
Merge branch 'xdmod11.0' into bar_chart_fix
aestoltm Dec 3, 2024
697a0c3
remove release note information
aestoltm Dec 3, 2024
ca3e2f4
Merge branch 'xdmod11.0' into bar_chart_fix
aestoltm Dec 13, 2024
42a4433
Realized was calculating the legend rank off by 1. Most likely didn't…
aestoltm Dec 13, 2024
b09a315
Merge branch 'xdmod11.0' into bar_chart_fix
aestoltm Dec 19, 2024
1c65d3f
Minor refactor to improve clarity
aestoltm Jan 2, 2025
a9f8ea3
Also refactor for export code
aestoltm Jan 2, 2025
a4fc2fc
Rename variable to be more precise about what it is
aestoltm Jan 2, 2025
124f845
Fix syntax error
aestoltm Jan 2, 2025
527afb2
Merge branch 'xdmod11.0' into bar_chart_fix
aestoltm Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions classes/DataWarehouse/Access/Usage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,11 +833,8 @@ function ($drillTarget) {
$chartSortedByValue
) {
// Determine the type of this data series.
$isTrendLineSeries = \xd_utilities\string_begins_with($meDataSeries['name'], 'Trend Line: ');
$isStdErrSeries = \xd_utilities\string_begins_with($meDataSeries['name'], 'Std Err: ');
$isNullSeries = $meDataSeries['name'] == 'gap connector';
$isPrimaryDataSeries = !($isTrendLineSeries || $isStdErrSeries || $isNullSeries);

$isPrimaryDataSeries = isset($meDataSeries['meta']['primarySeries']) && $meDataSeries['meta']['primarySeries'];
$isTrendLineSeries = isset($meDataSeries['meta']['trendlineSeries']) && $meDataSeries['meta']['trendlineSeries'];
// If this is a primary data series, increment the rank of the
// current primary data series. Further, if this chart is
// a timeseries chart, it is sorted by value, and it is a
Expand All @@ -848,7 +845,7 @@ function ($drillTarget) {
&& $chartSortedByValue
&& $usageGroupBy !== 'none'
) {
$rank = $meDataSeries['legendrank']+1;
$rank = $meDataSeries['legendrank'] / 3;
$meDataSeries['name'] = "${rank}. " . $meDataSeries['name'];
}
}
Expand Down
14 changes: 12 additions & 2 deletions classes/DataWarehouse/Visualization/AggregateChart.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ public function configure(

$yAxisArray = $this->setAxes($summarizeDataseries, $offset, $x_axis, $font_size);
$yAxisCount = count($yAxisArray);
$legendRank = 1;
$legendRank = 0;

// ------------ prepare to plot ------------

Expand Down Expand Up @@ -1110,6 +1110,9 @@ public function configure(
$trace = array_merge($trace, array(
'automargin'=> $data_description->display_type == 'pie' ? true : null,
'name' => $lookupDataSeriesName,
'meta' => array(
'primarySeries' => true
),
'customdata' => $lookupDataSeriesName,
'zIndex' => $zIndex,
'cliponaxis' => false,
Expand Down Expand Up @@ -1153,7 +1156,7 @@ public function configure(
'y' => $this->_swapXY ? $xValues : $yValues,
'drillable' => $drillable,
'yaxis' => "y{$yIndex}",
'offsetgroup' => $yIndex > 1 ? "group{$yIndex}" : "group{$legendRank}",
'offsetgroup' => $yAxisCount > 1 ? "group{$yIndex}{$legendRank}" : "group{$legendRank}",
'legendgroup' => $data_description_index,
'legendrank' => $legendRank - 1,
'traceorder' => $legendRank,
Expand All @@ -1172,6 +1175,9 @@ public function configure(
if($data_description->display_type!=='line')
{
if ($trace['type']=='area' && $data_description_index == 0) {
// "Area fix" trace is needed to have the ~5% padding on the
// left and right side of other plots. Otherwise the first and
// last values will be directly up to the end of the plotting area.
$hidden_trace = array(
'name' => 'area fix',
'x' => $this->_swapXY ? array_fill(0, count($xValues), 0) : $xValues,
Expand Down Expand Up @@ -1312,6 +1318,7 @@ public function configure(
),
'connectgaps' => true,
'hoverinfo' => 'skip',
'offsetgroup' => $yAxisCount > 1 ? "group{$yIndex}{$legendRank}" : "group{$legendRank}",
'legendgroup' => $data_description_index,
'legendrank' => -1000,
'traceorder' => -1000,
Expand Down Expand Up @@ -1465,6 +1472,9 @@ protected function buildErrorDataSeries(
// create the data series description:
$error_trace = array_merge($trace, array(
'name' => $lookupDataSeriesName,
'meta' => array(
'primarySeries' => false
),
'otitle' => $dsn,
'datasetId' => $data_description->id,
'color'=> $error_color,
Expand Down
27 changes: 17 additions & 10 deletions classes/DataWarehouse/Visualization/TimeseriesChart.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public function configure(
}

$yAxisCount = count($yAxisArray);

$legendRank = 0;
$globalFilterDescriptions = array();

// ==== Big long effing loop ====
Expand Down Expand Up @@ -420,6 +420,7 @@ public function configure(
// operate on each yAxisDataObject, a SimpleTimeseriesData object
foreach($yAxisDataObjectsArray as $traceIndex => $yAxisDataObject)
{
$legendRank += 3;
if( $yAxisDataObject != null)
{
$yAxisDataObject->joinTo($xAxisData, null);
Expand Down Expand Up @@ -566,6 +567,9 @@ public function configure(

$trace = array(
'name' => $lookupDataSeriesName,
'meta' => array(
'primarySeries' => true
),
'customdata' => $lookupDataSeriesName,
'otitle' => $formattedDataSeriesName,
'datasetId' => $data_description->id,
Expand Down Expand Up @@ -616,10 +620,10 @@ public function configure(
),
'x' => $this->_swapXY ? $yValues : $xValues,
'y' => $this->_swapXY ? $xValues : $yValues,
'offsetgroup' => $yIndex > 1 ? "group{$yIndex}" : "group{$traceIndex}",
'legendgroup' => $traceIndex,
'legendrank' => $traceIndex,
'traceorder' => $traceIndex,
'offsetgroup' => $yAxisCount > 1 ? "group{$yIndex}{$legendRank}" : "group{$legendRank}",
'legendgroup' => $legendRank,
'legendrank' => $legendRank,
'traceorder' => $legendRank,
'seriesData' => $seriesValues,
'visible' => $visible,
'isRemainder' => $isRemainder,
Expand Down Expand Up @@ -782,8 +786,8 @@ public function configure(
),
'connectgaps' => true,
'hoverinfo' => 'skip',
'offsetgroup' => $yIndex > 1 ? "group{$yIndex}" : "group{$traceIndex}",
'legendgroup' => $traceIndex,
'offsetgroup' => $yAxisCount > 1 ? "group{$yIndex}{$legendRank}" : "group{$legendRank}",
'legendgroup' => $legendRank,
'legendrank' => -1000,
'traceorder' => -1000,
'type' => 'scatter',
Expand Down Expand Up @@ -886,6 +890,10 @@ public function configure(
}
$trendline_trace = array(
'name' => $lookupDataSeriesName,
'meta' => array(
'primarySeries' => false,
'trendlineSeries' => true
),
'otitle' => $dsn,
'zIndex' => $zIndex,
'datasetId' => $data_description->id,
Expand All @@ -906,6 +914,8 @@ public function configure(
'color' => $color,
'width' => $data_description->line_width + $font_size/4,
),
'legendrank' => $trace['legendrank'] + 2,
'traceorder' => $trace['legendrank'] - 2,
'visible' => $visible,
'm' => $m,
'b' => $b,
Expand All @@ -914,9 +924,6 @@ public function configure(
'y' => $this->_swapXY ? $trendX : $trendY,
'isRestrictedByRoles' => $data_description->restrictedByRoles,
);
$valid = $data_description->std_err == 1 && !$data_description->log_scale;
$trendline_trace['legendrank'] = $valid ? $trace['legendrank'] + 2 : $trace['legendrank'];
$trendline_trace['traceorder'] = $valid ? $traceIndex - 2 : $traceIndex;

if ($this->_swapXY) {
unset($trendline_trace['yaxis']);
Expand Down
8 changes: 4 additions & 4 deletions html/gui/js/PlotlyChartWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ XDMoD.utils.createChart = function (chartOptions, extraHandlers) {
// Referenced https://stackoverflow.com/questions/45741397/javascript-sort-array-of-objects-by-2-properties
// for comparison idea
baseChartOptions.data.sort((trace1, trace2) => {
if (baseChartOptions.layout.barmode !== 'group') {
return Math.sign(trace2.zIndex - trace1.zIndex) || Math.sign(trace2.traceorder - trace1.traceorder);
const containsBarSeries = baseChartOptions.data.some((elem) => elem.type === 'bar');
if (containsBarSeries && baseChartOptions.layout.barmode === 'stack') {
return Math.sign(trace2.traceorder - trace1.traceorder);
Copy link
Member

Choose a reason for hiding this comment

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

Please change this if to explicitly check for a column bar chart (rather than not == group which will match scatter plots and pie charts too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be addressed.

For the trendline code that contained the ternaries, it seems that having the valid check was unnecessary. Nothing breaks when the log scale and std error bars are enabled with the trendline series. I need to start working on a few refactor PRs for Plotly because I saw a few cases where we can refactor barmode (only need for bar charts shouldn't be default on every chart) and allowing log scale with error bars (haven't found any reason why it shouldn't be allowed, I can look back on the git blame and try to find a reason)

}
const res = Math.sign(trace1.zIndex - trace2.zIndex) || Math.sign(trace1.traceorder - trace2.traceorder);
return res;
return Math.sign(trace1.traceorder - trace2.traceorder);
});
}

Expand Down
8 changes: 4 additions & 4 deletions html/plotly_template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,11 @@
}

chartOptions.data.sort((trace1, trace2) => {
if (chartOptions.layout.barmode !== 'group') {
return Math.sign(trace2.zIndex - trace1.zIndex) || Math.sign(trace2.traceorder - trace1.traceorder);
const containsBarSeries = chartOptions.data.some((elem) => elem.type === 'bar');
if (containsBarSeries && chartOptions.layout.barmode === 'stack') {
return Math.sign(trace2.traceorder - trace1.traceorder);
}
const res = Math.sign(trace1.zIndex - trace2.zIndex) || Math.sign(trace1.traceorder - trace2.traceorder);
return res;
return Math.sign(trace1.traceorder - trace2.traceorder);
});

}
Expand Down
Loading