Skip to content

Commit

Permalink
feat(trends): Add "Decimal places" as a Trends viz option (#19330)
Browse files Browse the repository at this point in the history
* feat(trends): Add "Decimal places" as a Trends viz option

* Do not round Trends numbers returned by the backend

* Unify default decimal places in the frontend

* Increase maximum places to 9

* Improve resiliency and update tests

* Update `filter_to_query`

* Only show "Decimal places" when fractional numbers are possible

* Update existing query tests for lack of server-side rounding
  • Loading branch information
Twixes authored Dec 15, 2023
1 parent 41160e3 commit 37d1954
Show file tree
Hide file tree
Showing 18 changed files with 116 additions and 15 deletions.
4 changes: 2 additions & 2 deletions frontend/src/lib/components/UnitPicker/UnitPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export function UnitPicker(): JSX.Element {
}, [localAxisFormat, trendsFilter])

return (
<div className="flex flex-1 pt-1.5 pb-2 pr-2">
<div className="flex-1 mb-2.5 mx-2">
<CustomUnitModal
formativeElement={customUnitModal}
isOpen={customUnitModal !== null}
Expand All @@ -92,12 +92,12 @@ export function UnitPicker(): JSX.Element {
overlayRef={(ref) => (customUnitModalRef.current = ref)}
/>
<LemonButtonWithDropdown
className="flex flex-1 ml-2"
onClick={() => setIsVisible(!isVisible)}
size={'small'}
type={'secondary'}
status="stealth"
data-attr="chart-aggregation-axis-format"
fullWidth
dropdown={{
onClickOutside: () => setIsVisible(false),
additionalRefs: [customUnitModalRef],
Expand Down
7 changes: 6 additions & 1 deletion frontend/src/lib/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -431,8 +431,13 @@ export function slugify(text: string): string {
.replace(/--+/g, '-')
}

export const DEFAULT_DECIMAL_PLACES = 2

/** Format number with comma as the thousands separator. */
export function humanFriendlyNumber(d: number, precision: number = 2): string {
export function humanFriendlyNumber(d: number, precision: number = DEFAULT_DECIMAL_PLACES): string {
if (isNaN(precision) || precision < 0) {
precision = DEFAULT_DECIMAL_PLACES
}
return d.toLocaleString('en-US', { maximumFractionDigits: precision })
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ describe('filtersToQueryNode', () => {
aggregation_axis_format: 'numeric',
aggregation_axis_prefix: '£',
aggregation_axis_postfix: '%',
decimal_places: 8,
breakdown_histogram_bin_count: 1,
formula: 'A+B',
shown_as: ShownAsValue.VOLUME,
Expand All @@ -321,6 +322,7 @@ describe('filtersToQueryNode', () => {
aggregation_axis_format: 'numeric',
aggregation_axis_prefix: '£',
aggregation_axis_postfix: '%',
decimal_places: 8,
formula: 'A+B',
display: ChartDisplayType.ActionsAreaGraph,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ export const filtersToQueryNode = (filters: Partial<FilterType>): InsightQueryNo
aggregation_axis_format: filters.aggregation_axis_format,
aggregation_axis_prefix: filters.aggregation_axis_prefix,
aggregation_axis_postfix: filters.aggregation_axis_postfix,
decimal_places: filters.decimal_places,
formula: filters.formula,
display: filters.display,
show_values_on_series: filters.show_values_on_series,
Expand Down
52 changes: 49 additions & 3 deletions frontend/src/queries/nodes/InsightViz/InsightDisplayConfig.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { LemonButton } from '@posthog/lemon-ui'
import { useValues } from 'kea'
import { LemonButton, LemonInput } from '@posthog/lemon-ui'
import { useActions, useValues } from 'kea'
import { ChartFilter } from 'lib/components/ChartFilter'
import { CompareFilter } from 'lib/components/CompareFilter/CompareFilter'
import { IntervalFilter } from 'lib/components/IntervalFilter'
Expand All @@ -8,6 +8,8 @@ import { UnitPicker } from 'lib/components/UnitPicker/UnitPicker'
import { FEATURE_FLAGS, NON_TIME_SERIES_DISPLAY_TYPES } from 'lib/constants'
import { LemonMenu, LemonMenuItems } from 'lib/lemon-ui/LemonMenu'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { DEFAULT_DECIMAL_PLACES } from 'lib/utils'
import posthog from 'posthog-js'
import { ReactNode } from 'react'
import { funnelDataLogic } from 'scenes/funnels/funnelDataLogic'
import { axisLabel } from 'scenes/insights/aggregationAxisFormat'
Expand All @@ -23,6 +25,7 @@ import { FunnelBinsPicker } from 'scenes/insights/views/Funnels/FunnelBinsPicker
import { FunnelDisplayLayoutPicker } from 'scenes/insights/views/Funnels/FunnelDisplayLayoutPicker'
import { PathStepPicker } from 'scenes/insights/views/Paths/PathStepPicker'
import { trendsDataLogic } from 'scenes/trends/trendsDataLogic'
import { useDebouncedCallback } from 'use-debounce'

import { ChartDisplayType } from '~/types'

Expand Down Expand Up @@ -62,7 +65,11 @@ export function InsightDisplayConfig(): JSX.Element {
(!display || display === ChartDisplayType.ActionsLineGraph) &&
featureFlags[FEATURE_FLAGS.SMOOTHING_INTERVAL]

const { showPercentStackView: isPercentStackViewOn, showValueOnSeries } = useValues(trendsDataLogic(insightProps))
const {
showPercentStackView: isPercentStackViewOn,
showValueOnSeries,
mightContainFractionalNumbers,
} = useValues(trendsDataLogic(insightProps))

const advancedOptions: LemonMenuItems = [
...(supportsValueOnSeries || showPercentStackView || hasLegend
Expand All @@ -85,6 +92,14 @@ export function InsightDisplayConfig(): JSX.Element {
},
]
: []),
...(mightContainFractionalNumbers && isTrends
? [
{
title: 'Decimal places',
items: [{ label: () => <DecimalPrecisionInput /> }],
},
]
: []),
]
const advancedOptionsCount: number =
(supportsValueOnSeries && showValueOnSeries ? 1 : 0) +
Expand Down Expand Up @@ -173,3 +188,34 @@ export function InsightDisplayConfig(): JSX.Element {
function ConfigFilter({ children }: { children: ReactNode }): JSX.Element {
return <span className="space-x-2 flex items-center text-sm">{children}</span>
}

function DecimalPrecisionInput(): JSX.Element {
const { insightProps } = useValues(insightLogic)
const { trendsFilter } = useValues(insightVizDataLogic(insightProps))
const { updateInsightFilter } = useActions(insightVizDataLogic(insightProps))

const reportChange = useDebouncedCallback(() => {
posthog.capture('decimal places changed', {
decimal_places: trendsFilter?.decimal_places,
})
}, 500)

return (
<LemonInput
type="number"
size="small"
step={1}
min={0}
max={9}
defaultValue={DEFAULT_DECIMAL_PLACES}
value={trendsFilter?.decimal_places}
onChange={(value) => {
updateInsightFilter({
decimal_places: value,
})
reportChange()
}}
className="mx-2 mb-1.5"
/>
)
}
3 changes: 3 additions & 0 deletions frontend/src/queries/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -3168,6 +3168,9 @@
"compare": {
"type": "boolean"
},
"decimal_places": {
"type": "number"
},
"display": {
"$ref": "#/definitions/ChartDisplayType"
},
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/scenes/insights/aggregationAxisFormat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const formatAggregationAxisValue = (
value: number | string
): string => {
value = Number(value)
let formattedValue = humanFriendlyNumber(value)
let formattedValue = humanFriendlyNumber(value, trendsFilter?.decimal_places)
if (trendsFilter?.aggregation_axis_format) {
switch (trendsFilter?.aggregation_axis_format) {
case 'duration':
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/scenes/insights/aggregationAxisFormats.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ describe('formatAggregationAxisValue', () => {
},
expected: '£3,940💖',
},
{ candidate: 0.8709423, filters: {}, expected: '0.87' },
{ candidate: 0.8709423, filters: { decimal_places: 2 }, expected: '0.87' },
{ candidate: 0.8709423, filters: { decimal_places: 3 }, expected: '0.871' },
{ candidate: 0.8709423, filters: { decimal_places: 9 }, expected: '0.8709423' },
{ candidate: 0.8709423, filters: { decimal_places: -1 }, expected: '0.87' }, // Fall back to default for unsupported values
]
formatTestcases.forEach((testcase) => {
it(`correctly formats "${testcase.candidate}" as ${testcase.expected} when filters are ${JSON.stringify(
Expand Down
1 change: 1 addition & 0 deletions frontend/src/scenes/insights/utils/compareInsightQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const cleanInsightQuery = (query: InsightQueryNode, ignoreVisualizationOnlyChang
aggregation_axis_format: undefined,
aggregation_axis_prefix: undefined,
aggregation_axis_postfix: undefined,
decimal_places: undefined,
layout: undefined,
toggledLifecycles: undefined,
}
Expand Down
36 changes: 35 additions & 1 deletion frontend/src/scenes/trends/trendsDataLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,30 @@ import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic'
import { keyForInsightLogicProps } from 'scenes/insights/sharedUtils'
import { BREAKDOWN_OTHER_NUMERIC_LABEL, BREAKDOWN_OTHER_STRING_LABEL } from 'scenes/insights/utils'

import { ChartDisplayType, InsightLogicProps, LifecycleToggle, TrendAPIResponse, TrendResult } from '~/types'
import { EntityNode } from '~/queries/schema'
import {
ChartDisplayType,
CountPerActorMathType,
HogQLMathType,
InsightLogicProps,
LifecycleToggle,
PropertyMathType,
TrendAPIResponse,
TrendResult,
} from '~/types'

import type { trendsDataLogicType } from './trendsDataLogicType'
import { IndexedTrendResult } from './types'

type MathType = Required<EntityNode>['math']

/** All math types that can result in non-whole numbers. */
const POSSIBLY_FRACTIONAL_MATH_TYPES: Set<MathType> = new Set(
[CountPerActorMathType.Average as MathType]
.concat(Object.values(HogQLMathType))
.concat(Object.values(PropertyMathType))
)

export const trendsDataLogic = kea<trendsDataLogicType>([
props({} as InsightLogicProps),
key(keyForInsightLogicProps('all_trends')),
Expand Down Expand Up @@ -156,6 +175,21 @@ export const trendsDataLogic = kea<trendsDataLogicType>([
() => [() => values.vizSpecificOptions],
(vizSpecificOptions) => vizSpecificOptions?.[ChartDisplayType.ActionsPie],
],

mightContainFractionalNumbers: [
(s) => [s.formula, s.series],
(formula, series): boolean => {
// Whether data points might contain fractional numbers, which involve extra display considerations,
// such as rounding
if (formula) {
return true
}
if (series) {
return series.some((s) => s.math && POSSIBLY_FRACTIONAL_MATH_TYPES.has(s.math))
}
return false
},
],
})),

listeners(({ actions, values }) => ({
Expand Down
1 change: 1 addition & 0 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,7 @@ export interface TrendsFilterType extends FilterType {
aggregation_axis_format?: AggregationAxisFormat // a fixed format like duration that needs calculation
aggregation_axis_prefix?: string // a prefix to add to the aggregation axis e.g. £
aggregation_axis_postfix?: string // a postfix to add to the aggregation axis e.g. %
decimal_places?: number
show_values_on_series?: boolean
show_labels_on_series?: boolean
show_percent_stack_view?: boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1069,7 +1069,7 @@ def test_trends_query_formula_rounding(self):
)

self.assertEqual(1, len(response.results))
self.assertEqual([0.33], response.results[0]["data"])
self.assertEqual([1 / 3], response.results[0]["data"])

@also_test_with_materialized_columns(["$some_property"])
def test_properties_filtering_with_materialized_columns_and_empty_string_as_property(self):
Expand Down
4 changes: 2 additions & 2 deletions posthog/hogql_queries/insights/trends/trends_query_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ def apply_formula(self, formula: str, results: List[Dict[str, Any]]) -> List[Dic
new_series_data = FormulaAST(series_data).call(formula)

new_result = group_list[0]
new_result["data"] = [round(value, 2) for value in new_series_data]
new_result["data"] = new_series_data
new_result["count"] = float(sum(new_series_data))
new_result["label"] = f"Formula ({formula})"

Expand All @@ -384,7 +384,7 @@ def apply_formula(self, formula: str, results: List[Dict[str, Any]]) -> List[Dic
new_series_data = FormulaAST(series_data).call(formula)
new_result = results[0]

new_result["data"] = [round(value, 2) for value in new_series_data]
new_result["data"] = new_series_data
new_result["count"] = float(sum(new_series_data))
new_result["label"] = f"Formula ({formula})"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ def _insight_filter(filter: Dict):
aggregation_axis_format=filter.get("aggregation_axis_format"),
aggregation_axis_prefix=filter.get("aggregation_axis_prefix"),
aggregation_axis_postfix=filter.get("aggregation_axis_postfix"),
decimal_places=filter.get("decimal_places"),
formula=filter.get("formula"),
display=clean_display(filter.get("display")),
show_values_on_series=filter.get("show_values_on_series"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1282,6 +1282,7 @@ def test_trends_filter(self):
"aggregation_axis_format": "duration_ms",
"aggregation_axis_prefix": "pre",
"aggregation_axis_postfix": "post",
"decimal_places": 5,
"formula": "A + B",
"shown_as": "Volume",
"display": "ActionsAreaGraph",
Expand All @@ -1299,6 +1300,7 @@ def test_trends_filter(self):
aggregation_axis_postfix="post",
formula="A + B",
display=ChartDisplayType.ActionsAreaGraph,
decimal_places=5,
),
)

Expand Down
3 changes: 1 addition & 2 deletions posthog/queries/trends/formula.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ def _run_formula_query(self, filter: Filter, team: Team):
additional_values["aggregated_value"] = item[1][0]
else:
additional_values["data"] = [
round(number, 2) if not math.isnan(number) and not math.isinf(number) else 0.0
for number in item[1]
number if not math.isnan(number) and not math.isinf(number) else 0.0 for number in item[1]
]
if filter.display == TRENDS_CUMULATIVE:
additional_values["data"] = list(accumulate(additional_values["data"]))
Expand Down
4 changes: 2 additions & 2 deletions posthog/queries/trends/test/test_formula.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,9 @@ def test_formula(self):
)
self.assertEqual(
self._run({"formula": "(A/3600)/B"})[0]["data"],
[0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0],
[0.0, 0.0, 0.0, 0.0, 0.0, 1 / 1200, 1 / 1800, 0.0],
)
self.assertEqual(self._run({"formula": "(A/3600)/B"})[0]["count"], 0)
self.assertEqual(self._run({"formula": "(A/3600)/B"})[0]["count"], 1 / 720)

self.assertEqual(
self._run({"formula": "A/0"})[0]["data"],
Expand Down
1 change: 1 addition & 0 deletions posthog/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,7 @@ class TrendsFilter(BaseModel):
aggregation_axis_prefix: Optional[str] = None
breakdown_histogram_bin_count: Optional[float] = None
compare: Optional[bool] = None
decimal_places: Optional[float] = None
display: Optional[ChartDisplayType] = None
formula: Optional[str] = None
hidden_legend_indexes: Optional[List[float]] = None
Expand Down

0 comments on commit 37d1954

Please sign in to comment.