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.

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

Merge release/v1.0.0 to develop #4867

Merged
merged 16 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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

Logical error: Redundant division in opacity calculation

In line 224, result is already normalized between 0 and 1 by dividing by max. Dividing result by max again in line 226 causes the opacity to be incorrectly reduced, which may result in the heatmap being rendered with unintended transparency levels.

Apply this diff to correct the calculation:

- return get32BitColor(color, result / max);
+ return get32BitColor(color, result);
📝 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
const result = Math.min(max, Math.abs(value)) / max;
return get32BitColor(color, value / max);
return get32BitColor(color, result / max);
const result = Math.min(max, Math.abs(value)) / 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
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 +68 to +74
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 clarity for clampedIndex function.

The test case covers important edge cases, but consider the following improvements:

  1. Add a test for a value within the heatmap range.
  2. Provide more descriptive comments explaining the function parameters.
  3. Consider using test.each for better readability and easier addition of more test cases.

Here's a suggested refactor:

describe("heatmap utils", () => {
  describe("clampedIndex", () => {
    test.each([
      { value: 1, min: 2, max: 3, steps: 4, expected: -1, description: "value below range" },
      { value: 4, min: 2, max: 3, steps: 4, expected: 3, description: "value above range" },
      { value: 2.5, min: 2, max: 3, steps: 4, expected: 2, description: "value within range" },
    ])("returns $expected when $description", ({ value, min, max, steps, expected }) => {
      expect(painter.clampedIndex(value, min, max, steps)).toBe(expected);
    });
  });
});

This refactor improves readability, adds a test for a value within the range, and makes it easier to add more test cases in the future.

});
52 changes: 36 additions & 16 deletions app/packages/looker/src/worker/painter.ts
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 +400 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 function

The clampedIndex function may cause a division by zero error when stop equals start. To prevent this, consider adding a condition to handle this case appropriately.

Apply this diff to handle the division by zero:

export const clampedIndex = (
  value: number,
  start: number,
  stop: number,
  length: number
) => {
  if (value < start) {
    return -1;
  }
+ if (stop === start) {
+   return length - 1; // Or handle as appropriate for your application
+ }
  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
) => {
if (value < start) {
return -1;
}
if (stop === start) {
return length - 1; // Or handle as appropriate for your application
}
const clamped = Math.min(value, stop);
return Math.round(
(Math.max(clamped - start, 0) / (stop - start)) * (length - 1)
);
};

Loading