Better heatmap range handling #4865
Conversation
WalkthroughThe changes involve significant updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/looker/src/overlays/heatmap.ts (3 hunks)
- app/packages/looker/src/worker/painter.test.ts (1 hunks)
- app/packages/looker/src/worker/painter.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/looker/src/overlays/heatmap.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/worker/painter.test.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/worker/painter.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (3)
app/packages/looker/src/overlays/heatmap.ts (1)
208-212: Correct use of 'clampedIndex' functionThe updated use of
clampedIndexensures that the index for color retrieval is properly clamped within the valid range, enhancing the robustness of the heatmap rendering.app/packages/looker/src/worker/painter.ts (2)
209-230: Improved handling of zero and out-of-range heatmap valuesThe updated logic correctly skips processing when
value === 0, and appropriately handles values outside the specified range using the newclampedIndexfunction. This ensures that heatmap rendering aligns with the PR objectives for better range handling.
394-396: Efficient iteration usingfor...ofloopReplacing the
forEachloop with afor...ofloop inconvertMaskColorsToObjectenhances performance, especially for large arrays. This change conforms with best practices in TypeScript for array iteration.
| 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); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ 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:
-
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
-
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)
-
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.
| const result = Math.min(max, Math.abs(value)) / max; | ||
|
|
||
| return get32BitColor(color, value / max); | ||
| return get32BitColor(color, result / max); |
There was a problem hiding this comment.
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);|
|
||
| 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) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
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.
| 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.
| 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) | |
| ); | |
| }; |
What changes are proposed in this pull request?
When heatmap values are above the
range, consider them themaxrange value. When they are below therange, consider them like a0with no coloring (background)Screen.Recording.2024-09-30.at.9.26.54.AM.mov
How is this patch tested? If it is not, please explain why.
Unit test
Release Notes
rangeattributeWhat areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
clampedIndexfunction to handle index calculations.clampedIndexfunction.Heatmapmethod and refined color assignment logic in theSegmentationmethod.