From f91bef64f163584e0cda7643e078bb1c94b99efa Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 8 May 2019 12:43:55 -0700 Subject: [PATCH] Stack chart functions in a more physical way Summary: Ref T13279. See that task for some discussion. The accumulations of some of the datasets may be negative (e.g., if more tasks are moved out of a project than into it) which can lead to negative area in the stacked chart. Introduce `min(...)` and `max(...)` to separate a function into points above or below some line, then mangle the areas to pick the negative and positive regions apart so they at least have a plausible physical interpretation and none of the areas are negative. This is presumably not a final version, I'm just trying to produce a chart that isn't a sequence of overlapping regions with negative areas that is "technically" correct but not really possible to interpret. Test Plan: {F6439195} Reviewers: amckinley Reviewed By: amckinley Subscribers: yelirekim Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20506 --- src/__phutil_library_map__.php | 4 ++ .../PhabricatorChartStackedAreaDataset.php | 8 ++- .../chart/PhabricatorMaxChartFunction.php | 40 ++++++++++++++ .../chart/PhabricatorMinChartFunction.php | 40 ++++++++++++++ .../fact/daemon/PhabricatorFactDaemon.php | 2 +- .../PhabricatorProjectBurndownChartEngine.php | 55 ++++++++++++++----- 6 files changed, 131 insertions(+), 18 deletions(-) create mode 100644 src/applications/fact/chart/PhabricatorMaxChartFunction.php create mode 100644 src/applications/fact/chart/PhabricatorMinChartFunction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3cdbaff8ec..428d4832b6 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3622,6 +3622,7 @@ phutil_register_library_map(array( 'PhabricatorMarkupInterface' => 'infrastructure/markup/PhabricatorMarkupInterface.php', 'PhabricatorMarkupOneOff' => 'infrastructure/markup/PhabricatorMarkupOneOff.php', 'PhabricatorMarkupPreviewController' => 'infrastructure/markup/PhabricatorMarkupPreviewController.php', + 'PhabricatorMaxChartFunction' => 'applications/fact/chart/PhabricatorMaxChartFunction.php', 'PhabricatorMemeEngine' => 'applications/macro/engine/PhabricatorMemeEngine.php', 'PhabricatorMemeRemarkupRule' => 'applications/macro/markup/PhabricatorMemeRemarkupRule.php', 'PhabricatorMentionRemarkupRule' => 'applications/people/markup/PhabricatorMentionRemarkupRule.php', @@ -3676,6 +3677,7 @@ phutil_register_library_map(array( 'PhabricatorMetronome' => 'infrastructure/util/PhabricatorMetronome.php', 'PhabricatorMetronomeTestCase' => 'infrastructure/util/__tests__/PhabricatorMetronomeTestCase.php', 'PhabricatorMetronomicTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorMetronomicTriggerClock.php', + 'PhabricatorMinChartFunction' => 'applications/fact/chart/PhabricatorMinChartFunction.php', 'PhabricatorModularTransaction' => 'applications/transactions/storage/PhabricatorModularTransaction.php', 'PhabricatorModularTransactionType' => 'applications/transactions/storage/PhabricatorModularTransactionType.php', 'PhabricatorMonogramDatasourceEngineExtension' => 'applications/typeahead/engineextension/PhabricatorMonogramDatasourceEngineExtension.php', @@ -9748,6 +9750,7 @@ phutil_register_library_map(array( 'PhabricatorMarkupInterface', ), 'PhabricatorMarkupPreviewController' => 'PhabricatorController', + 'PhabricatorMaxChartFunction' => 'PhabricatorChartFunction', 'PhabricatorMemeEngine' => 'Phobject', 'PhabricatorMemeRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorMentionRemarkupRule' => 'PhutilRemarkupRule', @@ -9814,6 +9817,7 @@ phutil_register_library_map(array( 'PhabricatorMetronome' => 'Phobject', 'PhabricatorMetronomeTestCase' => 'PhabricatorTestCase', 'PhabricatorMetronomicTriggerClock' => 'PhabricatorTriggerClock', + 'PhabricatorMinChartFunction' => 'PhabricatorChartFunction', 'PhabricatorModularTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorModularTransactionType' => 'Phobject', 'PhabricatorMonogramDatasourceEngineExtension' => 'PhabricatorDatasourceEngineExtension', diff --git a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php index 4a91aad635..8bf4445984 100644 --- a/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php +++ b/src/applications/fact/chart/PhabricatorChartStackedAreaDataset.php @@ -9,8 +9,10 @@ final class PhabricatorChartStackedAreaDataset PhabricatorChartDataQuery $data_query) { $functions = $this->getFunctions(); + $reversed_functions = array_reverse($functions, true); + $function_points = array(); - foreach ($functions as $function_idx => $function) { + foreach ($reversed_functions as $function_idx => $function) { $function_points[$function_idx] = array(); $datapoints = $function->newDatapoints($data_query); @@ -36,7 +38,7 @@ final class PhabricatorChartStackedAreaDataset } ksort($must_define); - foreach ($functions as $function_idx => $function) { + foreach ($reversed_functions as $function_idx => $function) { $missing = array(); foreach ($must_define as $x) { if (!isset($function_points[$function_idx][$x])) { @@ -136,6 +138,8 @@ final class PhabricatorChartStackedAreaDataset $series[] = $bounds; } + $series = array_reverse($series); + $events = array(); foreach ($raw_points as $function_idx => $points) { $event_list = array(); diff --git a/src/applications/fact/chart/PhabricatorMaxChartFunction.php b/src/applications/fact/chart/PhabricatorMaxChartFunction.php new file mode 100644 index 0000000000..c874cef8e8 --- /dev/null +++ b/src/applications/fact/chart/PhabricatorMaxChartFunction.php @@ -0,0 +1,40 @@ +newArgument() + ->setName('x') + ->setType('function'), + $this->newArgument() + ->setName('max') + ->setType('number'), + ); + } + + public function getDomain() { + return $this->getArgument('x')->getDomain(); + } + + public function newInputValues(PhabricatorChartDataQuery $query) { + return $this->getArgument('x')->newInputValues($query); + } + + public function evaluateFunction(array $xv) { + $yv = $this->getArgument('x')->evaluateFunction($xv); + $max = $this->getArgument('max'); + + foreach ($yv as $k => $y) { + if ($y > $max) { + $yv[$k] = null; + } + } + + return $yv; + } + +} diff --git a/src/applications/fact/chart/PhabricatorMinChartFunction.php b/src/applications/fact/chart/PhabricatorMinChartFunction.php new file mode 100644 index 0000000000..db1a003811 --- /dev/null +++ b/src/applications/fact/chart/PhabricatorMinChartFunction.php @@ -0,0 +1,40 @@ +newArgument() + ->setName('x') + ->setType('function'), + $this->newArgument() + ->setName('min') + ->setType('number'), + ); + } + + public function getDomain() { + return $this->getArgument('x')->getDomain(); + } + + public function newInputValues(PhabricatorChartDataQuery $query) { + return $this->getArgument('x')->newInputValues($query); + } + + public function evaluateFunction(array $xv) { + $yv = $this->getArgument('x')->evaluateFunction($xv); + $min = $this->getArgument('min'); + + foreach ($yv as $k => $y) { + if ($y < $min) { + $yv[$k] = null; + } + } + + return $yv; + } + +} diff --git a/src/applications/fact/daemon/PhabricatorFactDaemon.php b/src/applications/fact/daemon/PhabricatorFactDaemon.php index 57813b3021..29c7904547 100644 --- a/src/applications/fact/daemon/PhabricatorFactDaemon.php +++ b/src/applications/fact/daemon/PhabricatorFactDaemon.php @@ -15,7 +15,7 @@ final class PhabricatorFactDaemon extends PhabricatorDaemon { } $this->log(pht('Zzz...')); - $this->sleep(60 * 5); + $this->sleep(15); } } diff --git a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php index 35496330e8..092e921c5a 100644 --- a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php +++ b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php @@ -32,37 +32,62 @@ final class PhabricatorProjectBurndownChartEngine if ($project_phids) { foreach ($project_phids as $project_phid) { $function = $this->newFunction( - 'accumulate', - array('fact', 'tasks.open-count.create.project', $project_phid)); + 'min', + array( + 'accumulate', + array('fact', 'tasks.open-count.assign.project', $project_phid), + ), + 0); $function->getFunctionLabel() - ->setName(pht('Tasks Created')) - ->setColor('rgba(0, 0, 200, 1)') - ->setFillColor('rgba(0, 0, 200, 0.15)'); + ->setName(pht('Tasks Moved Into Project')) + ->setColor('rgba(0, 200, 200, 1)') + ->setFillColor('rgba(0, 200, 200, 0.15)'); $functions[] = $function; - $function = $this->newFunction( - 'accumulate', - array('fact', 'tasks.open-count.status.project', $project_phid)); + 'min', + array( + 'accumulate', + array('fact', 'tasks.open-count.status.project', $project_phid), + ), + 0); $function->getFunctionLabel() - ->setName(pht('Tasks Closed / Reopened')) + ->setName(pht('Tasks Reopened')) ->setColor('rgba(200, 0, 200, 1)') ->setFillColor('rgba(200, 0, 200, 0.15)'); $functions[] = $function; - $function = $this->newFunction( - 'accumulate', - array('fact', 'tasks.open-count.assign.project', $project_phid)); + 'sum', + array( + 'accumulate', + array('fact', 'tasks.open-count.create.project', $project_phid), + ), + array( + 'max', + array( + 'accumulate', + array('fact', 'tasks.open-count.status.project', $project_phid), + ), + 0, + ), + array( + 'max', + array( + 'accumulate', + array('fact', 'tasks.open-count.assign.project', $project_phid), + ), + 0, + )); $function->getFunctionLabel() - ->setName(pht('Tasks Rescoped')) - ->setColor('rgba(0, 200, 200, 1)') - ->setFillColor('rgba(0, 200, 200, 0.15)'); + ->setName(pht('Tasks Created')) + ->setColor('rgba(0, 0, 200, 1)') + ->setFillColor('rgba(0, 0, 200, 0.15)'); $functions[] = $function; }