rusackas commented on code in PR #37902:
URL: https://github.com/apache/superset/pull/37902#discussion_r2796447287
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,308 @@ 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)[];
-}
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+ 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;
+ });
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ 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],
+ );
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
+ 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);
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- 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
+ // Recompute table when data or control row changes, keeping control index
in range
+ useEffect(() => {
+ if (!data || data.length === 0) {
+ setControl(0);
+ setLiftValues([]);
+ setPValues([]);
+ return;
}
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
-
- 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 safeControlIndex = Math.min(control, data.length - 1);
+ if (safeControlIndex !== control) {
+ setControl(safeControlIndex);
+ computeTTest(safeControlIndex);
+ } else {
+ computeTTest(control);
}
+ }, [computeTTest, control, data]);
- return ''; // p-values won't normally be colored
- }
+ const getLiftStatus = useCallback(
+ (row: number): string => {
+ // 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
+ }
- 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';
- }
+ return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
Review Comment:
Fixed in 72ed19d325. Converted string values to numbers before applying
`Number.isNaN`/`Number.isFinite` checks:
```typescript
const numericLiftVal = Number(liftVal);
if (Number.isNaN(numericLiftVal) || \!Number.isFinite(numericLiftVal)) {
return 'invalid';
}
```
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,308 @@ 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)[];
-}
+function TTestTable({
+ alpha = 0.05,
+ data,
+ groups,
+ liftValPrec = 4,
+ metric,
+ pValPrec = 6,
+}: TTestTableProps) {
+ const [control, setControl] = useState(0);
+ const [liftValues, setLiftValues] = useState<(string | number)[]>([]);
+ const [pValues, setPValues] = useState<(string | number)[]>([]);
-const defaultProps = {
- alpha: 0.05,
- liftValPrec: 4,
- pValPrec: 6,
-};
+ 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;
+ });
-class TTestTable extends Component<TTestTableProps, TTestTableState> {
- static defaultProps = defaultProps;
+ return (((sumValues - sumControl) / sumControl) * 100).toFixed(
+ liftValPrec,
+ );
+ },
+ [liftValPrec],
+ );
- constructor(props: TTestTableProps) {
- super(props);
- this.state = {
- control: 0,
- liftValues: [],
- pValues: [],
- };
- }
+ 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],
+ );
- componentDidMount() {
- const { control } = this.state;
- this.computeTTest(control); // initially populate table
- }
+ 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);
+ setLiftValues(newLiftValues);
+ setPValues(newPValues);
+ },
+ [data, computeLift, computePValue],
+ );
- 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
+ // Recompute table when data or control row changes, keeping control index
in range
+ useEffect(() => {
+ if (!data || data.length === 0) {
+ setControl(0);
+ setLiftValues([]);
+ setPValues([]);
+ return;
}
- return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
- }
-
- 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 safeControlIndex = Math.min(control, data.length - 1);
+ if (safeControlIndex !== control) {
+ setControl(safeControlIndex);
+ computeTTest(safeControlIndex);
+ } else {
+ computeTTest(control);
}
+ }, [computeTTest, control, data]);
- return ''; // p-values won't normally be colored
- }
+ const getLiftStatus = useCallback(
+ (row: number): string => {
+ // 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
+ }
- 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';
- }
+ return Number(liftVal) >= 0 ? 'true' : 'false'; // green on true, red on
false
+ },
+ [control, liftValues],
+ );
- // p-values significant below set threshold
- return Number(pValues[row]) <= alpha;
- }
+ const getPValueStatus = useCallback(
+ (row: number): string => {
+ if (row === control) {
+ return 'control';
+ }
+ const pVal = pValues[row];
+ if (Number.isNaN(pVal) || !Number.isFinite(pVal)) {
Review Comment:
Fixed in 72ed19d325. Applied the same fix as `getLiftStatus` - converting
string to number before checks:
```typescript
const numericPVal = Number(pVal);
if (Number.isNaN(numericPVal) || \!Number.isFinite(numericPVal)) {
return 'invalid';
}
```
--
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]
