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 all 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
9 changes: 9 additions & 0 deletions app/packages/core/src/components/Modal/ImaVidLooker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -175,16 +175,23 @@ export const ImaVidLookerReact = React.memo(
return;
}

setPlayHeadState({ name: timelineName, state: "buffering" });

imaVidLookerRef.current.frameStoreController.enqueueFetch(
unprocessedBufferRange
);

imaVidLookerRef.current.frameStoreController.resumeFetch();

return new Promise<void>((resolve) => {
const fetchMoreListener = (e: CustomEvent) => {
if (
e.detail.id === imaVidLookerRef.current.frameStoreController.key
) {
if (storeBufferManager.containsRange(unprocessedBufferRange)) {
// todo: change playhead state in setFrameNumberAtom and not here
// if done here, store ref to last playhead status
setPlayHeadState({ name: timelineName, state: "paused" });
resolve();
window.removeEventListener(
"fetchMore",
Expand Down Expand Up @@ -250,6 +257,7 @@ export const ImaVidLookerReact = React.memo(
registerOnPauseCallback,
registerOnPlayCallback,
registerOnSeekCallbacks,
setPlayHeadState,
subscribe,
} = useCreateTimeline({
name: timelineName,
Expand Down Expand Up @@ -325,6 +333,7 @@ export const ImaVidLookerReact = React.memo(
position: "relative",
display: "flex",
flexDirection: "column",
overflowX: "hidden",
}}
>
<div
Expand Down
6 changes: 3 additions & 3 deletions app/packages/looker/src/elements/imavid/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,11 @@ export class ImaVidElement extends BaseElement<ImaVidState, HTMLImageElement> {
}

paintImageOnCanvas(image: HTMLImageElement) {
this.ctx.setTransform(1, 0, 0, 1, 0, 0);
this.ctx?.setTransform(1, 0, 0, 1, 0, 0);

this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height);
this.ctx?.clearRect(0, 0, this.canvas.width, this.canvas.height);

this.ctx.drawImage(image, 0, 0);
this.ctx?.drawImage(image, 0, 0);
}

async skipAndTryAgain(frameNumberToDraw: number, animate: boolean) {
Expand Down
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)
);
};

1 change: 1 addition & 0 deletions app/packages/playback/src/lib/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
} from "./constants";

export type PlayheadState =
| "buffering"
| "playing"
| "paused"
| "waitingToPlay"
Expand Down
12 changes: 12 additions & 0 deletions app/packages/playback/src/lib/use-create-timeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@ export const useCreateTimeline = (
return;
}

if (playHeadStateRef.current === "buffering") {
return;
}

setPlayHeadState({ name: timelineName, state: "playing" });
if (onPlayListenerRef.current) {
onPlayListenerRef.current();
Expand Down Expand Up @@ -364,6 +368,10 @@ export const useCreateTimeline = (
const key = e.key.toLowerCase();

if (key === " ") {
if (playHeadState === "buffering") {
return;
}

if (playHeadState === "paused") {
play();
} else {
Expand Down Expand Up @@ -448,6 +456,10 @@ export const useCreateTimeline = (
* Re-render all subscribers of the timeline with current frame number.
*/
refresh,
/**
* Set the playhead state of the timeline.
*/
setPlayHeadState,
/**
* Subscribe to the timeline.
*/
Expand Down
7 changes: 6 additions & 1 deletion app/packages/playback/src/views/svgs/buffering.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 2 additions & 6 deletions docs/scripts/make_model_zoo_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,9 @@
import fiftyone.zoo as foz
{% if 'segment-anything' in name and 'video' in name %}
from fiftyone import ViewField as F
{% endif %}

{% if 'med-sam' in name %}
{% elif 'med-sam' in name %}
from fiftyone import ViewField as F
from fiftyone.utils.huggingface import load_from_hub

dataset = load_from_hub("Voxel51/BTCV-CT-as-video-MedSAM2-dataset")[:2]
{% endif %}

{% if 'imagenet' in name %}
Expand All @@ -117,6 +113,7 @@
.save()
)
{% elif 'med-sam' in name %}
dataset = load_from_hub("Voxel51/BTCV-CT-as-video-MedSAM2-dataset")[:2]

# Retaining detections from a single frame in the middle
# Note that SAM2 only propagates segmentation masks forward in a video
Expand All @@ -126,7 +123,6 @@
.set_field("frames.gt_detections", None)
.save()
)

{% else %}
dataset = foz.load_zoo_dataset(
"coco-2017",
Expand Down
10 changes: 7 additions & 3 deletions docs/source/deprecation.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,23 @@ FiftyOne Deprecation Notices

.. default-role:: code

.. _deprecation-fiftyone-desktop:

FiftyOne Desktop
----------------
*Support ended with 0.25.0*
*Support ended with FiftyOne 0.25.0*

A compatible `fiftyone-desktop https://pypi.org/project/fiftyone-desktop/`_
A compatible `fiftyone-desktop <https://pypi.org/project/fiftyone-desktop>`_
package is no longer available as of `fiftyone==0.25.0`.

Chromium-based browsers, Firefox, or a :ref:`notebook <notebooks>` environment
are recommended for the best FiftyOne experience.

.. _deprecation-python-3.8:

Python 3.8
----------
*Support Ended October 2024*
*Support ended October 1, 2024*

`Python 3.8 <https://devguide.python.org/versions/>`_
transitions to `end-of-life` effective October of 2024. FiftyOne releases after
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
5 changes: 5 additions & 0 deletions docs/source/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ learn how:
:image_src: https://voxel51.com/images/integrations/v7-128.png
:image_title: V7

.. customimagelink::
:image_link: https://github.com/segments-ai/segments-voxel51-plugin
:image_src: https://voxel51.com/images/integrations/segments-128.png
:image_title: Segments

.. customimagelink::
:image_link: integrations/labelbox.html
:image_src: https://voxel51.com/images/integrations/labelbox-128.png
Expand Down
36 changes: 26 additions & 10 deletions docs/source/plugins/developing_plugins.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1774,12 +1774,11 @@ in the App.
Panels can be defined in either Python or JS, and FiftyOne comes with a
number of :ref:`builtin panels <plugins-design-panels>` for common tasks.

Depending on the ``surfaces`` panel config, panels can be scoped to either
the grid or the modal. You can open these panels from the "+" menu, which
is available in both the grid and modal views. Whereas grid panels enable
extensibility at the macro level, allowing you to work with entire datasets,
modal panels provide extensibility at the micro level, focusing on individual
samples and scenarios.
Panels can be scoped to the App's grid view or modal view via their
:ref:`config <panel-config>`. Grid panels enable extensibility at the macro
level, allowing you to work with entire datasets or views, while modal panels
provide extensibility at the micro level, focusing on individual samples and
scenarios.

Panels, like :ref:`operators <developing-operators>`, can make use of the
:mod:`fiftyone.operators.types` module and the
Expand Down Expand Up @@ -1838,10 +1837,9 @@ subsequent sections.
# Whether to allow multiple instances of the panel to be opened
allow_multiple=False,

# Whether the panel should be available in the grid view
# modal view, or both
# Whether the panel should be available in the grid, modal, or both
# Possible values: "grid", "modal", "grid modal"
surfaces="grid modal" # default = "grid"
surfaces="grid", # default = "grid"

# Markdown-formatted text that describes the panel. This is
# rendererd in a tooltip when the help icon in the panel
Expand Down Expand Up @@ -2138,7 +2136,7 @@ Panel config

Every panel must define a
:meth:`config <fiftyone.operators.panel.Panel.config>` property that
defines its name, display name, and other optional metadata about its
defines its name, display name, surfaces, and other optional metadata about its
behavior:

.. code-block:: python
Expand All @@ -2162,8 +2160,26 @@ behavior:

# Whether to allow multiple instances of the panel to be opened
allow_multiple=False,

# Whether the panel should be available in the grid, modal, or both
# Possible values: "grid", "modal", "grid modal"
surfaces="grid", # default = "grid"

# Markdown-formatted text that describes the panel. This is
# rendererd in a tooltip when the help icon in the panel
# title is hovered over
help_markdown="A description of the panel",
)

The ``surfaces`` key defines the panel's scope:

- Grid panels can be accessed from the ``+`` button in the App's
:ref:`grid view <app-fields-sidebar>`, which allows you to build macro
experiences that work with entire datasets or views
- Modal panels can be accessed from the ``+`` button in the App's
:ref:`modal view <app-sample-view>`, which allows you to build interactions
that focus on individual samples and scenarios

.. _panel-execution-context:

Execution context
Expand Down
Loading
Loading