rusackas commented on code in PR #39452: URL: https://github.com/apache/superset/pull/39452#discussion_r3534184009
########## superset-frontend/plugins/legacy-plugin-chart-horizon/test/HorizonRow.test.tsx: ########## @@ -0,0 +1,102 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import HorizonRow from '../src/HorizonRow'; + +const mockContext = { + imageSmoothingEnabled: false, + fillStyle: '', + clearRect: jest.fn(), + setTransform: jest.fn(), + translate: jest.fn(), + scale: jest.fn(), + fillRect: jest.fn(), +}; + +beforeAll(() => { + jest + .spyOn(HTMLCanvasElement.prototype, 'getContext') + .mockReturnValue(mockContext as unknown as CanvasRenderingContext2D); +}); Review Comment: Added an afterAll to restore the spy, thanks. ########## superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx: ########## @@ -32,279 +32,324 @@ export interface DataEntry { } interface TTestTableProps { - alpha: number; + alpha?: number; data: DataEntry[]; groups: string[]; - liftValPrec: number; + liftValPrec?: number; metric: string; - pValPrec: number; + pValPrec?: number; } -interface TTestTableState { - control: number; - liftValues: (string | number)[]; - pValues: (string | number)[]; -} - -const defaultProps = { - alpha: 0.05, - liftValPrec: 4, - pValPrec: 6, -}; +function TTestTable({ + alpha = 0.05, + data, + groups, + liftValPrec = 4, + metric, + pValPrec = 6, +}: TTestTableProps) { + const [control, setControl] = useState(0); + // Mirrors `control` so the data-change effect can read the latest value + // without re-running (and recomputing) after every row click + const controlRef = useRef(0); + const [liftValues, setLiftValues] = useState<(string | number)[]>([]); + const [pValues, setPValues] = useState<(string | number)[]>([]); -class TTestTable extends Component<TTestTableProps, TTestTableState> { - static defaultProps = defaultProps; - - constructor(props: TTestTableProps) { - super(props); - this.state = { - control: 0, - liftValues: [], - pValues: [], - }; - } + const computeLift = useCallback( + (values: DataPointValue[], controlValues: DataPointValue[]): string => { + // Compute the lift value between two time series + let sumValues = 0; + let sumControl = 0; + values.forEach((value, i) => { + sumValues += value.y; + sumControl += controlValues[i].y; + }); - componentDidMount() { - const { control } = this.state; - this.computeTTest(control); // initially populate table - } - - getLiftStatus(row: number): string { - const { control, liftValues } = this.state; - // Get a css class name for coloring - if (row === control) { - return 'control'; - } - const liftVal = liftValues[row]; - if (Number.isNaN(liftVal) || !Number.isFinite(liftVal)) { - return 'invalid'; // infinite or NaN values - } - - return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on false - } + // A zero control sum yields "Infinity" (or "NaN" for 0/0) via toFixed, + // matching the original class component's output + return (((sumValues - sumControl) / sumControl) * 100).toFixed( + liftValPrec, + ); + }, + [liftValPrec], + ); - getPValueStatus(row: number): string { - const { control, pValues } = this.state; - if (row === control) { - return 'control'; - } - const pVal = pValues[row]; - if (Number.isNaN(pVal) || !Number.isFinite(pVal)) { - return 'invalid'; - } + const computePValue = useCallback( + ( + values: DataPointValue[], + controlValues: DataPointValue[], + ): string | number => { + // Compute the p-value from Student's t-test + // between two time series + let diffSum = 0; + let diffSqSum = 0; + let finiteCount = 0; + values.forEach((value, i) => { + const diff = controlValues[i].y - value.y; + /* eslint-disable-next-line */ + if (isFinite(diff)) { + finiteCount += 1; + diffSum += diff; + diffSqSum += diff * diff; + } + }); + const tvalue = -Math.abs( + diffSum * + Math.sqrt( + (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum), + ), + ); + try { + return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed( + pValPrec, + ); // two-sided test + } catch (error) { + return NaN; + } + }, + [pValPrec], + ); - return ''; // p-values won't normally be colored - } + const computeTTest = useCallback( + (controlIndex: number) => { + // Compute lift and p-values for each row + // against the selected control + const newPValues: (string | number)[] = []; + const newLiftValues: (string | number)[] = []; + if (!data) { + return; + } + for (let i = 0; i < data.length; i += 1) { + if (i === controlIndex) { + newPValues.push('control'); + newLiftValues.push('control'); + } else { + newPValues.push( + computePValue(data[i].values, data[controlIndex].values), + ); + newLiftValues.push( + computeLift(data[i].values, data[controlIndex].values), + ); + } + } + setControl(controlIndex); + controlRef.current = controlIndex; + setLiftValues(newLiftValues); + setPValues(newPValues); + }, + [data, computeLift, computePValue], + ); - getSignificance(row: number): string | boolean { - const { control, pValues } = this.state; - const { alpha } = this.props; - // Color significant as green, else red - if (row === control) { - return 'control'; + // Recompute table when data changes, keeping control index in range. + // Row clicks call computeTTest directly, so `control` is read via a ref + // here to avoid a duplicate recompute after each click. + useEffect(() => { + if (!data || data.length === 0) { + setControl(0); + controlRef.current = 0; + setLiftValues([]); + setPValues([]); + return; } - // p-values significant below set threshold - return Number(pValues[row]) <= alpha; - } + computeTTest(Math.min(controlRef.current, data.length - 1)); + }, [computeTTest, data]); - computeLift(values: DataPointValue[], control: DataPointValue[]): string { - const { liftValPrec } = this.props; - // Compute the lift value between two time series - let sumValues = 0; - let sumControl = 0; - values.forEach((value, i) => { - sumValues += value.y; - sumControl += control[i].y; - }); + const getLiftStatus = useCallback( + (row: number): string => { + // Get a css class name for coloring + if (row === control) { + return 'control'; + } + const liftVal = liftValues[row]; + const numericLiftVal = Number(liftVal); + if (Number.isNaN(numericLiftVal) || !Number.isFinite(numericLiftVal)) { + return 'invalid'; // infinite or NaN values + } - return (((sumValues - sumControl) / sumControl) * 100).toFixed(liftValPrec); - } + return numericLiftVal >= 0 ? 'true' : 'false'; // green on true, red on false + }, + [control, liftValues], + ); - computePValue( - values: DataPointValue[], - control: DataPointValue[], - ): string | number { - const { pValPrec } = this.props; - // Compute the p-value from Student's t-test - // between two time series - let diffSum = 0; - let diffSqSum = 0; - let finiteCount = 0; - values.forEach((value, i) => { - const diff = control[i].y - value.y; - /* eslint-disable-next-line */ - if (isFinite(diff)) { - finiteCount += 1; - diffSum += diff; - diffSqSum += diff * diff; + const getPValueStatus = useCallback( + (row: number): string => { + if (row === control) { + return 'control'; + } + const pVal = pValues[row]; + const numericPVal = Number(pVal); + if (Number.isNaN(numericPVal) || !Number.isFinite(numericPVal)) { + return 'invalid'; } - }); - const tvalue = -Math.abs( - diffSum * - Math.sqrt( - (finiteCount - 1) / (finiteCount * diffSqSum - diffSum * diffSum), - ), - ); - try { - return (2 * new dist.Studentt(finiteCount - 1).cdf(tvalue)).toFixed( - pValPrec, - ); // two-sided test - } catch (error) { - return NaN; - } - } - computeTTest(control: number) { - // Compute lift and p-values for each row - // against the selected control - const { data } = this.props; - const pValues: (string | number)[] = []; - const liftValues: (string | number)[] = []; - if (!data) { - return; - } - for (let i = 0; i < data.length; i += 1) { - if (i === control) { - pValues.push('control'); - liftValues.push('control'); - } else { - pValues.push(this.computePValue(data[i].values, data[control].values)); - liftValues.push(this.computeLift(data[i].values, data[control].values)); + return ''; // p-values won't normally be colored + }, + [control, pValues], + ); + + const getSignificance = useCallback( + (row: number): string | boolean => { + // Color significant as green, else red + if (row === control) { + return 'control'; } - } - this.setState({ control, liftValues, pValues }); - } - render() { - const { data, metric, groups } = this.props; - const { control, liftValues, pValues } = this.state; + // p-values significant below set threshold + return Number(pValues[row]) <= alpha; + }, + [control, pValues, alpha], + ); - if (!Array.isArray(groups) || groups.length === 0) { - throw new Error('Group by param is required'); - } + const handleRowClick = useCallback( + (rowIndex: number) => { + computeTTest(rowIndex); + }, + [computeTTest], + ); - // Render column header for each group - const columns = groups.map((group, i) => ( - <Th key={i} column={group}> - {group} - </Th> - )); - const numGroups = groups.length; - // Columns for p-value, lift-value, and significance (true/false) - columns.push( - <Th key={numGroups + 1} column="pValue"> - p-value - </Th>, - ); - columns.push( - <Th key={numGroups + 2} column="liftValue"> - Lift % - </Th>, - ); - columns.push( - <Th key={numGroups + 3} column="significant"> - Significant - </Th>, - ); - const rows = data.map((entry, i) => { - const values = groups.map( - ( - group, - j, // group names - ) => <Td key={j} column={group} data={entry.group[j]} />, - ); - values.push( - <Td - key={numGroups + 1} - className={this.getPValueStatus(i)} - column="pValue" - data={pValues[i]} - />, - ); - values.push( - <Td - key={numGroups + 2} - className={this.getLiftStatus(i)} - column="liftValue" - data={liftValues[i]} - />, - ); - values.push( - <Td - key={numGroups + 3} - className={this.getSignificance(i).toString()} - column="significant" - data={this.getSignificance(i)} - />, - ); + // When sorted ascending, 'control' will always be at top + type SortConfigItem = + string | { column: string; sortFunction: (a: string, b: string) => number }; - return ( - <Tr - key={i} - className={i === control ? 'control' : ''} - onClick={this.computeTTest.bind(this, i)} - > - {values} - </Tr> - ); - }); - // When sorted ascending, 'control' will always be at top - type SortConfigItem = - | string - | { column: string; sortFunction: (a: string, b: string) => number }; - const sortConfig: SortConfigItem[] = (groups as SortConfigItem[]).concat([ - { - column: 'pValue', - sortFunction: (a: string, b: string) => { - if (a === 'control') { - return -1; - } - if (b === 'control') { - return 1; - } + const sortConfig: SortConfigItem[] = useMemo( + () => + (groups as SortConfigItem[]).concat([ + { + column: 'pValue', + sortFunction: (a: string, b: string) => { + if (a === 'control') { + return -1; + } + if (b === 'control') { + return 1; + } + if (a === b) { + return 0; + } - return a > b ? 1 : -1; // p-values ascending + return a > b ? 1 : -1; // p-values ascending + }, }, - }, - { - column: 'liftValue', - sortFunction: (a: string, b: string) => { - if (a === 'control') { - return -1; - } - if (b === 'control') { - return 1; - } + { + column: 'liftValue', + sortFunction: (a: string, b: string) => { + if (a === 'control') { + return -1; + } + if (b === 'control') { + return 1; + } - return parseFloat(a) > parseFloat(b) ? -1 : 1; // lift values descending + const liftA = parseFloat(a); + const liftB = parseFloat(b); + if (liftA === liftB) { + return 0; + } + + return liftA > liftB ? -1 : 1; // lift values descending Review Comment: Added explicit non-finite checks so it returns 0/-1/1 deterministically instead of leaning on NaN comparisons. Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
