Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better heatmap range handling #4865

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Better heatmap range handling #4865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7a3f3e
ensure heatmap value is within range
benjaminpkane Sep 23, 2024
3bb8574
add test
benjaminpkane Sep 27, 2024
7e82c80
lint
benjaminpkane Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions app/packages/looker/src/overlays/heatmap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ import {
import { ARRAY_TYPES, OverlayMask, TypedArray } from "../numpy";
import { BaseState, Coordinates } from "../state";
import { isFloatArray } from "../util";
import { clampedIndex } from "../worker/painter";
import {
BaseLabel,
CONTAINS,
isShown,
Overlay,
PointInfo,
SelectData,
isShown,
} from "./base";
import { sizeBytes, strokeCanvasRect, t } from "./util";

Expand Down Expand Up @@ -204,12 +205,13 @@ export default class HeatmapOverlay<State extends BaseState>
}

if (state.options.coloring.by === "value") {
const index = Math.round(
(Math.max(value - start, 0) / (stop - start)) *
(state.options.coloring.scale.length - 1)
const index = clampedIndex(
value,
start,
stop,
state.options.coloring.scale.length
);

return get32BitColor(state.options.coloring.scale[index]);
return index < 0 ? 0 : get32BitColor(state.options.coloring.scale[index]);
}

const color = getColor(
Expand All @@ -219,9 +221,9 @@ export default class HeatmapOverlay<State extends BaseState>
);
const max = Math.max(Math.abs(start), Math.abs(stop));

value = Math.min(max, Math.abs(value)) / max;
const result = Math.min(max, Math.abs(value)) / max;

return get32BitColor(color, value / max);
return get32BitColor(color, result / max);
Comment on lines +224 to +226
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Possible double division by 'max' in alpha computation

In the calculation of result, you already divide by max:

const result = Math.min(max, Math.abs(value)) / max;

Then in the return statement, you're dividing result by max again:

return get32BitColor(color, result / max);

This means you're dividing by max twice, which may not be intended and could result in incorrect alpha values for the color.

Apply this diff to correct the calculation:

- return get32BitColor(color, result / max);
+ return get32BitColor(color, result);

}

private getTarget(state: Readonly<State>): number {
Expand Down
10 changes: 10 additions & 0 deletions app/packages/looker/src/worker/painter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,13 @@ describe("filter resolves correctly", () => {
).toBeUndefined();
});
});

describe("heatmap utils", () => {
it("clamps for heatmaps", async () => {
// A value below a heatmap range returns -1
expect(painter.clampedIndex(1, 2, 3, 4)).toBe(-1);

// A value above a heatmap range return the max
expect(painter.clampedIndex(4, 2, 3, 4)).toBe(3);
});
});
Comment on lines +67 to +75
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage and improve test structure

The new test suite for the clampedIndex function is a good addition. However, consider the following improvements:

  1. Split the current test case into two separate test cases for better clarity and easier debugging:

    • One for testing values below the range
    • Another for testing values above the range

  2. Add more test cases to cover:

    • Edge cases (e.g., values exactly at the min and max of the range)
    • Values within the range
    • Invalid inputs (if applicable)

  3. Make the test descriptions more specific about what is being tested.

Here's an example of how you could restructure the tests:

describe("heatmap utils", () => {
  describe("clampedIndex", () => {
    it("returns -1 for values below the range", () => {
      expect(painter.clampedIndex(1, 2, 3, 4)).toBe(-1);
    });

    it("returns the max index for values above the range", () => {
      expect(painter.clampedIndex(4, 2, 3, 4)).toBe(3);
    });

    it("returns the correct index for values within the range", () => {
      expect(painter.clampedIndex(2.5, 2, 3, 4)).toBe(1); // Assuming linear scale
    });

    it("handles edge cases correctly", () => {
      expect(painter.clampedIndex(2, 2, 3, 4)).toBe(0);
      expect(painter.clampedIndex(3, 2, 3, 4)).toBe(3);
    });
  });
});

This structure provides better organization and more comprehensive coverage of the clampedIndex function's behavior.

52 changes: 36 additions & 16 deletions app/packages/looker/src/worker/painter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,23 +206,28 @@ export const PainterFactory = (requestColor) => ({
}

// 0 is background image
if (value !== 0) {
let r;
if (coloring.by === COLOR_BY.FIELD) {
color =
fieldSetting?.fieldColor ??
(await requestColor(coloring.pool, coloring.seed, field));

r = get32BitColor(color, Math.min(max, Math.abs(value)) / max);
} else {
const index = Math.round(
(Math.max(value - start, 0) / (stop - start)) * (scale.length - 1)
);
r = get32BitColor(scale[index]);
if (value === 0) {
continue;
}
let r: number;
if (coloring.by === COLOR_BY.FIELD) {
color =
fieldSetting?.fieldColor ??
(await requestColor(coloring.pool, coloring.seed, field));

r = get32BitColor(color, Math.min(max, Math.abs(value)) / max);
} else {
const index = clampedIndex(value, start, stop, scale.length);

if (index < 0) {
// values less than range start are background
continue;
}

overlay[i] = r;
r = get32BitColor(scale[index]);
}

overlay[i] = r;
}
},
Segmentation: async (
Expand Down Expand Up @@ -386,8 +391,23 @@ export const convertToHex = (color: string) =>
const convertMaskColorsToObject = (array: MaskColorInput[]) => {
const result = {};
if (!array) return {};
array.forEach((item) => {
for (const item of array) {
result[item.intTarget.toString()] = item.color;
});
}
return result;
};

export const clampedIndex = (
value: number,
start: number,
stop: number,
length: number
) => {
if (value < start) {
return -1;
}
const clamped = Math.min(value, stop);
return Math.round(
(Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
);
};
Comment on lines +399 to +413
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Potential division by zero in clampedIndex when start equals stop

In the clampedIndex function, if start equals stop, the expression stop - start will be zero, leading to a division by zero error. This could cause a runtime exception or unexpected behavior.

Apply this diff to prevent division by zero:

 export const clampedIndex = (
   value: number,
   start: number,
   stop: number,
   length: number
+ ): number => {
+  if (stop === start) {
+    return -1;
+  }
   if (value < start) {
     return -1;
   }
   const clamped = Math.min(value, stop);
   return Math.round(
     (Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
   );
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const clampedIndex = (
value: number,
start: number,
stop: number,
length: number
) => {
if (value < start) {
return -1;
}
const clamped = Math.min(value, stop);
return Math.round(
(Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
);
};
export const clampedIndex = (
value: number,
start: number,
stop: number,
length: number
): number => {
if (stop === start) {
return -1;
}
if (value < start) {
return -1;
}
const clamped = Math.min(value, stop);
return Math.round(
(Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
);
};

🛠️ Refactor suggestion

Specify return types for exported functions

For better type safety and clarity, it's recommended to explicitly specify the return types of exported functions in TypeScript.

Add explicit return types to the function declarations:

 export const clampedIndex = (
   value: number,
   start: number,
   stop: number,
   length: number
- ) => {
+ ): number => {

Similarly, for convertMaskColorsToObject:

 const convertMaskColorsToObject = (array: MaskColorInput[])
- => {
+ : { [key: string]: string } => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const clampedIndex = (
value: number,
start: number,
stop: number,
length: number
) => {
if (value < start) {
return -1;
}
const clamped = Math.min(value, stop);
return Math.round(
(Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
);
};
export const clampedIndex = (
value: number,
start: number,
stop: number,
length: number
): number => {
if (value < start) {
return -1;
}
const clamped = Math.min(value, stop);
return Math.round(
(Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
);
};

Loading