rusackas commented on code in PR #39452:
URL: https://github.com/apache/superset/pull/39452#discussion_r3532971586
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx:
##########
@@ -114,35 +107,37 @@ const StyledDiv = styled.div`
`}
`;
-class PairedTTest extends PureComponent<PairedTTestProps> {
- static defaultProps = defaultProps;
-
- render() {
- const { className, metrics, groups, data, alpha, pValPrec, liftValPrec } =
- this.props;
-
- return (
- <StyledDiv>
- <div className={`superset-legacy-chart-paired-t-test ${className}`}>
- <div className="paired-ttest-table">
- <div className="scrollbar-content">
- {metrics.map((metric, i) => (
- <TTestTable
- key={i}
- metric={metric}
- groups={groups}
- data={data[metric]}
- alpha={alpha}
- pValPrec={Math.min(pValPrec, 32)}
- liftValPrec={Math.min(liftValPrec, 32)}
- />
- ))}
- </div>
+function PairedTTest({
+ alpha = 0.05,
+ className = '',
+ data,
+ groups,
+ liftValPrec = 4,
+ metrics,
+ pValPrec = 6,
+}: PairedTTestProps) {
+ return (
+ <StyledDiv>
+ <div className={`superset-legacy-chart-paired-t-test ${className}`}>
Review Comment:
Good catch, class name mismatch was there before this PR too. Fixed by
lining up the selector with the hyphenated className.
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,325 @@ 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;
+ });
Review Comment:
This matches the original class component's behavior, not something this PR
introduced. Keeping it out of scope here since this PR's just the FC
conversion, not a behavior fix.
##########
superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/TTestTable.tsx:
##########
@@ -32,279 +32,325 @@ 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;
+ }
+ });
Review Comment:
Same as the other one, this is preexisting behavior carried over from the
class component. Out of scope for a pure conversion PR.
--
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]
