quickstart 3d dataset #4406
quickstart 3d dataset #4406
Conversation
WalkthroughThe updates introduce a new 3D dataset called "Quickstart 3D" and enhance orthographic projection functionality. Key changes include adding a new dataset class, updating documentation, and modifying the Looker package to handle 3D data more effectively. These improvements support better visualization, projection, and handling of 3D datasets, enriching the user experience with 3D data in FiftyOne. 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
fiftyone/zoo/datasets/base.py (1)
Line range hint
3359-3382: Replace bareexceptclauses with specific exceptions in_should_patch_pcd.The method
_should_patch_pcduses multiple bareexceptclauses. This practice can hide bugs and make the code harder to maintain. It's better to catch specific exceptions or at least log the exception details.- except: + except KeyError as e: + logger.error("Key error: %s", str(e)) + except FileNotFoundError as e: + logger.error("File not found: %s", str(e))
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- docs/source/user_guide/dataset_zoo/datasets.rst (1 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
Files skipped from review due to trivial changes (1)
- docs/source/user_guide/dataset_zoo/datasets.rst
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3359-3359: Do not use bare
except
3367-3367: Do not use bare
except
3375-3375: Do not use bare
except
3382-3382: Do not use bare
except
Additional comments not posted (1)
fiftyone/zoo/datasets/base.py (1)
3221-3237: Replace bareexceptclauses with specific exceptions.The method
_download_and_prepareuses bareexceptclauses, which can obscure the source of errors and make debugging more challenging. Consider specifying the exceptions you expect to handle or add logging to capture more information about unexpected errors.- except: + except Exception as e: + logger.error("An error occurred: %s", str(e))Likely invalid or redundant comment.
fiftyone/zoo/datasets/base.py
Outdated
| class Quickstart3DDataset(FiftyOneDataset): | ||
| """A small 3D dataset with meshes, point clouds, and oriented bounding boxes. | ||
|
|
||
| The dataset consists of 200 3D mesh samples from the test split of the | ||
| `ModelNet40 <https://modelnet.cs.princeton.edu>`_ dataset, with point | ||
| clouds generated using a Poisson disk sampling method, and oriented | ||
| bounding boxes generated based on the convex hull. | ||
|
|
||
| Objects have been rescaled and recentered from the original dataset. | ||
|
|
||
| Example usage:: | ||
|
|
||
| import fiftyone as fo | ||
| import fiftyone.zoo as foz | ||
|
|
||
| dataset = foz.load_zoo_dataset("quickstart-3d") | ||
|
|
||
| session = fo.launch_app(dataset) | ||
|
|
||
| Dataset size | ||
| 223.40 MB | ||
| """ | ||
|
|
||
| _GDRIVE_ID = "1EnQ2-gGDktEd8pAWwdXNK-FeHUFTFl5K" | ||
| _ARCHIVE_NAME = "quickstart-3d.zip" | ||
| _DIR_IN_ARCHIVE = "quickstart-3d" | ||
|
|
||
| @property | ||
| def name(self): | ||
| return "quickstart-3d" | ||
|
|
||
| @property | ||
| def tags(self): | ||
| return ("3d", "point-cloud", "mesh", "quickstart") | ||
|
|
||
| @property | ||
| def supported_splits(self): | ||
| return None | ||
|
|
||
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | ||
| _download_and_extract_archive( | ||
| self._GDRIVE_ID, | ||
| self._ARCHIVE_NAME, | ||
| self._DIR_IN_ARCHIVE, | ||
| dataset_dir, | ||
| scratch_dir, | ||
| ) | ||
|
|
||
| logger.info("Parsing dataset metadata") | ||
| dataset_type = fot.FiftyOneDataset() | ||
| importer = foud.FiftyOneDatasetImporter | ||
| num_samples = importer._get_num_samples(dataset_dir) | ||
| logger.info("Found %d samples", num_samples) | ||
|
|
||
| return dataset_type, num_samples, None | ||
|
|
There was a problem hiding this comment.
Add comprehensive docstrings for methods and properties.
The class Quickstart3DDataset lacks detailed docstrings for its methods and properties, which are essential for understanding the functionality and usage of these components. Consider adding docstrings similar to those in other dataset classes in this file.
sashankaryal
force-pushed
the
quickstart-3d
branch
from
60eab97 to
f76b955
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
app/packages/looker-3d/src/fo3d/Gizmos.tsx (1)
21-21: Consider adding a comment explaining the choice ofGRID_SECTION_COLOR.Adding a comment about why this specific color was chosen could improve code readability and maintainability.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
e2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/3d/fo3d-pcd-stl.spec.ts-snapshots/scene-chromium-linux.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (4)
- app/packages/looker-3d/src/fo3d/Gizmos.tsx (5 hunks)
- app/packages/looker-3d/src/state.ts (1 hunks)
- docs/source/user_guide/dataset_zoo/datasets.rst (1 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/looker-3d/src/state.ts
Files skipped from review as they are similar to previous changes (1)
- docs/source/user_guide/dataset_zoo/datasets.rst
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3359-3359: Do not use bare
except
3367-3367: Do not use bare
except
3375-3375: Do not use bare
except
3382-3382: Do not use bare
except
Path-based Instructions (1)
app/packages/looker-3d/src/fo3d/Gizmos.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Learnings (1)
app/packages/looker-3d/src/fo3d/Gizmos.tsx (1)
User: sashankaryal PR: voxel51/fiftyone#4310 File: app/packages/looker-3d/src/fo3d/Gizmos.tsx:110-0 Timestamp: 2024-04-25T21:03:45.341Z Learning: The grid rendering logic in `Gizmos.tsx` is part of an external component and not directly modifiable within the project's codebase.
Additional comments not posted (4)
app/packages/looker-3d/src/fo3d/Gizmos.tsx (3)
123-139: Change fromuseMemotouseEffectfor setting grid sizes is appropriate.Using
useEffecthere is suitable as it handles side effects which depend on the changingsceneSize. This ensures that the grid sizes are recalculated only when necessary.
116-117: Proper use ofuseRecoilStatefor dynamic grid size management.The use of
useRecoilStateforgridCellSizeAtomandgridSectionSizeAtomis appropriate as it allows both reading and updating the state, which is necessary for dynamically setting grid sizes based on the scene dimensions.
Line range hint
2-165: Overall structure and logic of theGizmoscomponent are well-implemented.The component effectively integrates state management and conditional rendering to provide a dynamic and responsive 3D grid and axis helpers. It adheres to React and Recoil best practices.
fiftyone/zoo/datasets/base.py (1)
3182-3237: Add comprehensive docstrings for methods and properties.
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | ||
| _download_and_extract_archive( | ||
| self._GDRIVE_ID, | ||
| self._ARCHIVE_NAME, | ||
| self._DIR_IN_ARCHIVE, | ||
| dataset_dir, | ||
| scratch_dir, | ||
| ) |
There was a problem hiding this comment.
Ensure proper error handling in the _download_and_prepare method.
- _download_and_extract_archive(
- self._GDRIVE_ID,
- self._ARCHIVE_NAME,
- self._DIR_IN_ARCHIVE,
- dataset_dir,
- scratch_dir,
- )
+ try:
+ _download_and_extract_archive(
+ self._GDRIVE_ID,
+ self._ARCHIVE_NAME,
+ self._DIR_IN_ARCHIVE,
+ dataset_dir,
+ scratch_dir,
+ )
+ except Exception as e:
+ logger.error("Failed to download or extract archive: %s", e)
+ raiseCommittable 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.
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | |
| _download_and_extract_archive( | |
| self._GDRIVE_ID, | |
| self._ARCHIVE_NAME, | |
| self._DIR_IN_ARCHIVE, | |
| dataset_dir, | |
| scratch_dir, | |
| ) | |
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | |
| try: | |
| _download_and_extract_archive( | |
| self._GDRIVE_ID, | |
| self._ARCHIVE_NAME, | |
| self._DIR_IN_ARCHIVE, | |
| dataset_dir, | |
| scratch_dir, | |
| ) | |
| except Exception as e: | |
| logger.error("Failed to download or extract archive: %s", e) | |
| raise |
| const nominalCellSize = maxDim / 10; | ||
|
|
||
| // round to the nearest 10, but for small values, just use 1 | ||
| const roundedCellSize = | ||
| nominalCellSize <= 1 ? 1 : Math.ceil(nominalCellSize / 10) * 10; | ||
| const roundedSectionSize = roundedCellSize * 10; |
There was a problem hiding this comment.
Refactor magic numbers into named constants for clarity.
- const nominalCellSize = maxDim / 10;
- const roundedCellSize = nominalCellSize <= 1 ? 1 : Math.ceil(nominalCellSize / 10) * 10;
- const roundedSectionSize = roundedCellSize * 10;
+ const CELL_SIZE_DIVISOR = 10;
+ const SECTION_SIZE_MULTIPLIER = 10;
+ const nominalCellSize = maxDim / CELL_SIZE_DIVISOR;
+ const roundedCellSize = nominalCellSize <= 1 ? 1 : Math.ceil(nominalCellSize / CELL_SIZE_DIVISOR) * CELL_SIZE_DIVISOR;
+ const roundedSectionSize = roundedCellSize * SECTION_SIZE_MULTIPLIER;Refactoring these magic numbers into named constants improves code readability and maintainability.
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.
| const nominalCellSize = maxDim / 10; | |
| // round to the nearest 10, but for small values, just use 1 | |
| const roundedCellSize = | |
| nominalCellSize <= 1 ? 1 : Math.ceil(nominalCellSize / 10) * 10; | |
| const roundedSectionSize = roundedCellSize * 10; | |
| const CELL_SIZE_DIVISOR = 10; | |
| const SECTION_SIZE_MULTIPLIER = 10; | |
| const nominalCellSize = maxDim / CELL_SIZE_DIVISOR; | |
| // round to the nearest 10, but for small values, just use 1 | |
| const roundedCellSize = | |
| nominalCellSize <= 1 ? 1 : Math.ceil(nominalCellSize / CELL_SIZE_DIVISOR) * CELL_SIZE_DIVISOR; | |
| const roundedSectionSize = roundedCellSize * SECTION_SIZE_MULTIPLIER; |
| const [cellSize, setCellSize] = useRecoilState(gridCellSizeAtom); | ||
| const [sectionSize, setSectionSize] = useRecoilState(gridSectionSizeAtom); |
There was a problem hiding this comment.
Tip
Codebase Verification
The removal of the effects property from gridCellSizeAtom and gridSectionSizeAtom means that the grid settings will no longer be persisted across sessions. This change could negatively impact the user experience by failing to retain grid settings.
-
gridCellSizeAtomdefinition:export const gridCellSizeAtom = atom<number>({ key: "fo3d-gridCellSize", default: 1, });
-
gridSectionSizeAtomdefinition:export const gridSectionSizeAtom = atom<number>({ key: "fo3d-gridSectionSize", default: 10, });
Please ensure that the removal of the effects property does not negatively impact the user experience by failing to persist grid settings across sessions.
Analysis chain
Verify the impact of removing browser storage effects from grid size atoms.
Please ensure that the removal of the effects property does not negatively impact the user experience by failing to persist grid settings across sessions.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the removal of browser storage effects affects the persistence of grid settings across sessions.
# Test: Search for usage of `gridCellSizeAtom` and `gridSectionSizeAtom` in the codebase.
rg --type typescript $'gridCellSizeAtom|gridSectionSizeAtom'
Length of output: 96
Script:
#!/bin/bash
# Description: Check if the removal of browser storage effects affects the persistence of grid settings across sessions.
# Test: Search for usage of `gridCellSizeAtom` and `gridSectionSizeAtom` in the codebase.
rg --type ts 'gridCellSizeAtom|gridSectionSizeAtom'
Length of output: 978
Script:
#!/bin/bash
# Description: Extract the definitions of `gridCellSizeAtom` and `gridSectionSizeAtom` to review their current state.
# Extract the definition of `gridCellSizeAtom`.
rg --type ts --context 5 'export const gridCellSizeAtom' app/packages/looker-3d/src/state.ts
# Extract the definition of `gridSectionSizeAtom`.
rg --type ts --context 5 'export const gridSectionSizeAtom' app/packages/looker-3d/src/state.ts
Length of output: 635
sashankaryal
force-pushed
the
quickstart-3d
branch
from
f76b955 to
0f63fe2
Compare
sashankaryal
force-pushed
the
quickstart-3d
branch
from
0f63fe2 to
ccc8309
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
fiftyone/zoo/datasets/base.py (1)
Line range hint
636-636: Avoid using bareexceptstatements.- except: + except Exception as e: + logger.error("An error occurred: %s", e)This change ensures that exceptions are caught more explicitly, and relevant errors are logged, improving the robustness and maintainability of the error handling.
Also applies to: 643-643, 3359-3359, 3367-3367, 3375-3375, 3382-3382
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- docs/source/user_guide/dataset_zoo/datasets.rst (1 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/source/user_guide/dataset_zoo/datasets.rst
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3359-3359: Do not use bare
except
3367-3367: Do not use bare
except
3375-3375: Do not use bare
except
3382-3382: Do not use bare
except
Additional comments not posted (2)
fiftyone/zoo/datasets/base.py (2)
3182-3237: Add comprehensive docstrings for methods and properties.
3221-3228: Ensure proper error handling in the_download_and_preparemethod.
improve 3d bbox projection logic
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
fiftyone/utils/utils3d.py (1)
429-430: Ensure thenormalfield is properly documented.The addition of the
normalfield in theOrthographicProjectionMetadataclass is crucial for handling 3D projections. It would be beneficial to expand the documentation to explain the purpose and expected format of this field to aid future developers and users of this API.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
app/packages/looker/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/*.lock,!**/*.lock
Files selected for processing (7)
- app/packages/looker/src/overlays/detection.ts (2 hunks)
- app/packages/looker/src/state.ts (1 hunks)
- app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1 hunks)
- app/packages/looker/src/worker/label-3d-projection-utils.ts (1 hunks)
- app/packages/looker/src/worker/threed-label-processor.ts (5 hunks)
- fiftyone/utils/utils3d.py (4 hunks)
- tests/unittests/utils3d_tests.py (2 hunks)
Additional Context Used
Biome (36)
app/packages/looker/src/overlays/detection.ts (8)
195-195: The assignment should not be in an expression.
204-204: The assignment should not be in an expression.
292-301: Prefer for...of instead of forEach.
6-7: All these imports are only used as types.
7-8: All these imports are only used as types.
9-10: Some named imports are only used as types.
194-194: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
201-201: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
app/packages/looker/src/state.ts (10)
120-120: Unexpected any. Specify a different type.
129-129: This enum declaration contains members that are implicitly initialized.
187-187: Unexpected any. Specify a different type.
322-322: Unexpected any. Specify a different type.
411-411: Unexpected any. Specify a different type.
493-493: Unexpected any. Specify a different type.
4-5: All these imports are only used as types.
5-6: All these imports are only used as types.
6-7: All these imports are only used as types.
8-9: All these imports are only used as types.
app/packages/looker/src/worker/label-3d-projection-utils.test.ts (1)
1-2: Some named imports are only used as types.
app/packages/looker/src/worker/threed-label-processor.ts (17)
147-147: Declare variables separately
147-147: This variable implicitly has the any type.
201-203: Prefer for...of instead of forEach.
1-1: Some named imports are only used as types.
3-4: All these imports are only used as types.
4-5: All these imports are only used as types.
5-9: Some named imports are only used as types.
37-37: Use Number.Infinity instead of the equivalent global.
38-38: Use Number.Infinity instead of the equivalent global.
39-39: Use Number.Infinity instead of the equivalent global.
40-40: Use Number.Infinity instead of the equivalent global.
70-70: Use Number.Infinity instead of the equivalent global.
71-71: Use Number.Infinity instead of the equivalent global.
74-74: Use Number.Infinity instead of the equivalent global.
77-77: Use Number.Infinity instead of the equivalent global.
83-83: Use Number.Infinity instead of the equivalent global.
86-86: Use Number.Infinity instead of the equivalent global.
Path-based Instructions (5)
app/packages/looker/src/worker/label-3d-projection-utils.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/label-3d-projection-utils.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/threed-label-processor.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/overlays/detection.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/state.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 not posted (6)
app/packages/looker/src/worker/label-3d-projection-utils.ts (2)
26-35: TheprojectTo2Dfunction is well-implemented for projecting points onto specified planes.
19-24: Ensure proper handling of Euler angles and coordinate systems when applying rotations.Verification successful
The
rotatePointfunction is defined and used within the same file. The implementation involves creating aVector3andEulerfrom the input points and applying the rotation. This approach appears to handle Euler angles and coordinate systems correctly.
Implementation:
export const rotatePoint = (point: Vec3, rotation: Vec3): Vec3 => { const threePoint = new Vector3(...point); const threeRotation = new Euler(...rotation); return threePoint.applyEuler(threeRotation).toArray() as Vec3; };Usage:
const rotated = rotatePoint(corner, newRotation);Based on the extracted information, the rotation logic seems to be correctly implemented.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the rotation logic by comparing with known transformations. # Test: Check if the rotation logic matches expected results for known Euler angles. rg --type typescript $'rotatePoint'Length of output: 69
Script:
#!/bin/bash # Description: Verify the correctness of the rotation logic by comparing with known transformations. # Test: Check if the rotation logic matches expected results for known Euler angles. rg --type ts 'rotatePoint'Length of output: 268
Script:
#!/bin/bash # Description: Extract the implementation and usage context of `rotatePoint`. # Extract the implementation of `rotatePoint`. ast-grep --lang typescript --pattern $'export const rotatePoint = ($_): $_ => { $$$ }' > rotatePoint_implementation.txt # Extract the usage context of `rotatePoint`. rg 'rotatePoint' > rotatePoint_usage.txtLength of output: 106
Script:
#!/bin/bash # Description: Extract the implementation and usage context of `rotatePoint`. # Extract the implementation of `rotatePoint`. ast-grep --lang typescript --pattern 'export const rotatePoint = $_' > rotatePoint_implementation.txt # Extract the usage context of `rotatePoint`. rg 'rotatePoint' > rotatePoint_usage.txtLength of output: 87
Script:
#!/bin/bash # Description: Extract the implementation and usage context of `rotatePoint`. # Extract the implementation and usage context of `rotatePoint` from the file. sed -n '/rotatePoint/p' app/packages/looker/src/worker/label-3d-projection-utils.tsLength of output: 205
app/packages/looker/src/overlays/detection.ts (1)
24-24: Ensure that theconvexHullproperty is properly documented and tested.app/packages/looker/src/state.ts (1)
68-70: The updatedOrthographicProjectionMetadatatype now correctly includes 3D bounds and a normal vector.tests/unittests/utils3d_tests.py (1)
Line range hint
288-308: The unit tests correctly validate the newnormalfield inOrthographicProjectionMetadata.fiftyone/utils/utils3d.py (1)
907-907: Ensure consistency in metadata handling.It's important to verify that the
normalfield is consistently handled across all functions that utilize theOrthographicProjectionMetadataclass. This script checks for the usage of thenormalfield wherever the metadata class is instantiated or manipulated.
| if projection_normal is None: | ||
| projection_normal = [0, 0, 1] | ||
|
|
There was a problem hiding this comment.
Set default projection_normal earlier to simplify logic.
- if projection_normal is None:
- projection_normal = [0, 0, 1]
+ projection_normal = projection_normal or [0, 0, 1]Refactoring the assignment of the default value for projection_normal simplifies the code and reduces the need for a separate conditional block.
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.
| if projection_normal is None: | |
| projection_normal = [0, 0, 1] | |
| projection_normal = projection_normal or [0, 0, 1] |
fiftyone/utils/utils3d.py
Outdated
| @@ -435,6 +437,7 @@ | |||
| filepath = fof.StringField() | |||
| min_bound = fof.ListField(fof.FloatField()) | |||
| max_bound = fof.ListField(fof.FloatField()) | |||
| normal = fof.ListField(fof.FloatField()) | |||
There was a problem hiding this comment.
Validate the normal field to ensure it always contains three elements.
- normal = fof.ListField(fof.FloatField())
+ normal = fof.ListField(fof.FloatField(), min_length=3, max_length=3)The normal field should always contain exactly three elements to represent a 3D vector. The proposed change ensures that the data integrity is maintained by enforcing this constraint at the database level.
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.
| normal = fof.ListField(fof.FloatField()) | |
| normal = fof.ListField(fof.FloatField(), min_length=3, max_length=3) |
| import { describe, expect, it } from "vitest"; | ||
| import { Vec3, projectTo2D } from "./label-3d-projection-utils"; |
There was a problem hiding this comment.
Consider using type-only imports for Vec3 and projectTo2D if they are only used as types.
- import { Vec3, projectTo2D } from "./label-3d-projection-utils";
+ import type { Vec3, projectTo2D } from "./label-3d-projection-utils";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.
| import { describe, expect, it } from "vitest"; | |
| import { Vec3, projectTo2D } from "./label-3d-projection-utils"; | |
| import { describe, expect, it } from "vitest"; | |
| import type { Vec3, projectTo2D } from "./label-3d-projection-utils"; |
| export const getProjectedCorners = ( | ||
| box: BoundingBox3D, | ||
| plane: "xz" | "xy" | "yz" | ||
| ) => { | ||
| const { dimensions, location, rotation } = box; | ||
| const [dx, dy, dz] = dimensions; | ||
| const halfDimensions = [dx / 2, dy / 2, dz / 2] as Vec3; | ||
|
|
||
| // Generate the 8 corners of the 3D bounding box | ||
| const corners: Vec3[] = [ | ||
| // left bottom back | ||
| [-halfDimensions[0], -halfDimensions[1], -halfDimensions[2]], | ||
| // left bottom front | ||
| [-halfDimensions[0], -halfDimensions[1], halfDimensions[2]], | ||
| // left top back | ||
| [-halfDimensions[0], halfDimensions[1], -halfDimensions[2]], | ||
| // left top front | ||
| [-halfDimensions[0], halfDimensions[1], halfDimensions[2]], | ||
| // right bottom back | ||
| [halfDimensions[0], -halfDimensions[1], -halfDimensions[2]], | ||
| // right bottom front | ||
| [halfDimensions[0], -halfDimensions[1], halfDimensions[2]], | ||
| // right top back | ||
| [halfDimensions[0], halfDimensions[1], -halfDimensions[2]], | ||
| // right top front | ||
| [halfDimensions[0], halfDimensions[1], halfDimensions[2]], | ||
| ]; | ||
|
|
||
| // rotate first, and translate | ||
| const transformedCorners = corners.map((corner) => { | ||
| const newRotation = rotation; | ||
|
|
||
| const rotated = rotatePoint(corner, newRotation); | ||
| return [ | ||
| rotated[0] + location[0], | ||
| rotated[1] + location[1], | ||
| rotated[2] + location[2], | ||
| ] as Vec3; | ||
| }); | ||
|
|
||
| // project the 3D points to 2D based on the specified plane | ||
| const projectedCorners: Vec2[] = transformedCorners.map((corner) => | ||
| projectTo2D(corner, plane) | ||
| ); | ||
|
|
||
| return { projectedCorners }; | ||
| }; |
There was a problem hiding this comment.
Optimize the corner transformation and projection logic by precomputing repeated operations.
- const transformedCorners = corners.map((corner) => {
- const newRotation = rotation;
- const rotated = rotatePoint(corner, newRotation);
- return [
- rotated[0] + location[0],
- rotated[1] + location[1],
- rotated[2] + location[2],
- ] as Vec3;
- });
+ const rotatedCorners = corners.map(corner => rotatePoint(corner, rotation));
+ const transformedCorners = rotatedCorners.map(rotated => [
+ rotated[0] + location[0],
+ rotated[1] + location[1],
+ rotated[2] + location[2],
+ ] as Vec3);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.
| export const getProjectedCorners = ( | |
| box: BoundingBox3D, | |
| plane: "xz" | "xy" | "yz" | |
| ) => { | |
| const { dimensions, location, rotation } = box; | |
| const [dx, dy, dz] = dimensions; | |
| const halfDimensions = [dx / 2, dy / 2, dz / 2] as Vec3; | |
| // Generate the 8 corners of the 3D bounding box | |
| const corners: Vec3[] = [ | |
| // left bottom back | |
| [-halfDimensions[0], -halfDimensions[1], -halfDimensions[2]], | |
| // left bottom front | |
| [-halfDimensions[0], -halfDimensions[1], halfDimensions[2]], | |
| // left top back | |
| [-halfDimensions[0], halfDimensions[1], -halfDimensions[2]], | |
| // left top front | |
| [-halfDimensions[0], halfDimensions[1], halfDimensions[2]], | |
| // right bottom back | |
| [halfDimensions[0], -halfDimensions[1], -halfDimensions[2]], | |
| // right bottom front | |
| [halfDimensions[0], -halfDimensions[1], halfDimensions[2]], | |
| // right top back | |
| [halfDimensions[0], halfDimensions[1], -halfDimensions[2]], | |
| // right top front | |
| [halfDimensions[0], halfDimensions[1], halfDimensions[2]], | |
| ]; | |
| // rotate first, and translate | |
| const transformedCorners = corners.map((corner) => { | |
| const newRotation = rotation; | |
| const rotated = rotatePoint(corner, newRotation); | |
| return [ | |
| rotated[0] + location[0], | |
| rotated[1] + location[1], | |
| rotated[2] + location[2], | |
| ] as Vec3; | |
| }); | |
| // project the 3D points to 2D based on the specified plane | |
| const projectedCorners: Vec2[] = transformedCorners.map((corner) => | |
| projectTo2D(corner, plane) | |
| ); | |
| return { projectedCorners }; | |
| }; | |
| // rotate first, and translate | |
| const rotatedCorners = corners.map(corner => rotatePoint(corner, rotation)); | |
| const transformedCorners = rotatedCorners.map(rotated => [ | |
| rotated[0] + location[0], | |
| rotated[1] + location[1], | |
| rotated[2] + location[2], | |
| ] as Vec3); |
| const { min_bound, max_bound, normal } = orthographicProjectionParams; | ||
| const [xmin, ymin, zmin] = min_bound; | ||
| const [xmax, ymax, zmax] = max_bound; | ||
|
|
||
| const [lx, ly, lz] = label.location; // centroid of bounding box | ||
| const [dx, dy, dz] = label.dimensions; // length of bounding box in each dimension | ||
| const [rx, ry, rz] = label.rotation ?? [0, 0, 0]; // rotation of bounding box | ||
|
|
||
| const [nx, ny, nz] = normal ?? [0, 0, 1]; | ||
|
|
||
| const box: BoundingBox3D = { | ||
| dimensions: [dx, dy, dz], | ||
| location: [lx, ly, lz], | ||
| rotation: [rx, ry, rz], | ||
| }; | ||
|
|
||
| let projectionPlane: "xy" | "xz" | "yz" = "xy"; | ||
|
|
||
| if (nx === 1 || nx === -1) { | ||
| // project on yz plane | ||
| projectionPlane = "yz"; | ||
| } else if (ny === 1 || ny === -1) { | ||
| // project on xz plane | ||
| projectionPlane = "xz"; | ||
| } else if (nz === 1 || nz === -1) { | ||
| // project on xy plane | ||
| projectionPlane = "xy"; | ||
| } | ||
|
|
||
| const { projectedCorners } = getProjectedCorners(box, projectionPlane); | ||
|
|
||
| const xRange = xmax - xmin; | ||
| const yRange = ymax - ymin; | ||
| const zRange = zmax - zmin; | ||
|
|
||
| const newProjectedCorners = projectedCorners.map(([x, y]) => { | ||
| let px, py; | ||
|
|
||
| // todo: need to account for negative / positive normals | ||
| switch (projectionPlane) { | ||
| case "xy": | ||
| px = (x - xmin) / xRange; | ||
| py = (ymax - y) / yRange; | ||
| break; | ||
| case "xz": | ||
| px = (x - xmin) / xRange; | ||
| py = (zmax - y) / zRange; | ||
| break; | ||
| case "yz": | ||
| px = (y - ymin) / yRange; | ||
| py = (zmax - x) / zRange; | ||
| break; | ||
| } | ||
| return [px, py]; | ||
| }); |
There was a problem hiding this comment.
Refactor the Detection method to improve readability and maintainability.
- const { min_bound, max_bound, normal } = orthographicProjectionParams;
- const [xmin, ymin, zmin] = min_bound;
- const [xmax, ymax, zmax] = max_bound;
- const [lx, ly, lz] = label.location; // centroid of bounding box
- const [dx, dy, dz] = label.dimensions; // length of bounding box in each dimension
- const [rx, ry, rz] = label.rotation ?? [0, 0, 0]; // rotation of bounding box
- const [nx, ny, nz] = normal ?? [0, 0, 1];
- const box: BoundingBox3D = {
- dimensions: [dx, dy, dz],
- location: [lx, ly, lz],
- rotation: [rx, ry, rz],
- };
- let projectionPlane: "xy" | "xz" | "yz" = "xy";
- if (nx === 1 || nx === -1) {
- projectionPlane = "yz";
- } else if (ny === 1 || ny === -1) {
- projectionPlane = "xz";
- } else if (nz === 1 || nz === -1) {
- projectionPlane = "xy";
- }
- const { projectedCorners } = getProjectedCorners(box, projectionPlane);
- const xRange = xmax - xmin;
- const yRange = ymax - ymin;
- const zRange = zmax - zmin;
- const newProjectedCorners = projectedCorners.map(([x, y]) => {
- let px, py;
- switch (projectionPlane) {
- case "xy":
- px = (x - xmin) / xRange;
- py = (ymax - y) / yRange;
- break;
- case "xz":
- px = (x - xmin) / xRange;
- py = (zmax - y) / zRange;
- break;
- case "yz":
- px = (y - ymin) / yRange;
- py = (zmax - x) / zRange;
- break;
- }
- return [px, py];
- });
+ // Simplified and refactored code hereCommittable suggestion was skipped due low confidence.
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
docs/source/user_guide/using_datasets.rst (1)
Line range hint
4709-4714: Ensure thecompute_orthographic_projection_imagesfunction call is correctly documented.The function call to
compute_orthographic_projection_imagesis missing some parameters in the documentation. It's important to provide complete and accurate documentation for function calls, especially for utility functions that users are likely to reuse. This helps prevent errors and confusion when others try to use this function.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
docs/source/images/dataset_zoo/quickstart-3d.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (3)
- docs/source/user_guide/dataset_zoo/datasets.rst (2 hunks)
- docs/source/user_guide/using_datasets.rst (4 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/source/user_guide/dataset_zoo/datasets.rst
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3360-3360: Do not use bare
except
3368-3368: Do not use bare
except
3376-3376: Do not use bare
except
3383-3383: Do not use bare
except
| Orthographic projection images currently only include point clouds, not | ||
| meshes or 3D shapes. | ||
|
|
There was a problem hiding this comment.
Clarify the scope of the orthographic projection images limitation.
The note mentions that orthographic projection images currently only include point clouds and not meshes or 3D shapes. It would be beneficial to clarify whether this is a temporary limitation and if there are any plans or methods to include meshes and 3D shapes in the future. This information can help users better understand the current capabilities and future developments.
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | ||
| _download_and_extract_archive( | ||
| self._GDRIVE_ID, | ||
| self._ARCHIVE_NAME, | ||
| self._DIR_IN_ARCHIVE, | ||
| dataset_dir, | ||
| scratch_dir, | ||
| ) | ||
|
|
||
| logger.info("Parsing dataset metadata") | ||
| dataset_type = fot.FiftyOneDataset() | ||
| importer = foud.FiftyOneDatasetImporter | ||
| num_samples = importer._get_num_samples(dataset_dir) | ||
| logger.info("Found %d samples", num_samples) | ||
|
|
||
| return dataset_type, num_samples, None |
There was a problem hiding this comment.
Specify exception types in try-except blocks to avoid suppressing unexpected errors.
- try:
- _download_and_extract_archive(
- self._GDRIVE_ID,
- self._ARCHIVE_NAME,
- self._DIR_IN_ARCHIVE,
- dataset_dir,
- scratch_dir,
- )
- except:
- logger.error("Failed to download or extract archive")
- raise
+ try:
+ _download_and_extract_archive(
+ self._GDRIVE_ID,
+ self._ARCHIVE_NAME,
+ self._DIR_IN_ARCHIVE,
+ dataset_dir,
+ scratch_dir,
+ )
+ except Exception as e:
+ logger.error("Failed to download or extract archive: %s", e)
+ raiseCommittable 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.
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | |
| _download_and_extract_archive( | |
| self._GDRIVE_ID, | |
| self._ARCHIVE_NAME, | |
| self._DIR_IN_ARCHIVE, | |
| dataset_dir, | |
| scratch_dir, | |
| ) | |
| logger.info("Parsing dataset metadata") | |
| dataset_type = fot.FiftyOneDataset() | |
| importer = foud.FiftyOneDatasetImporter | |
| num_samples = importer._get_num_samples(dataset_dir) | |
| logger.info("Found %d samples", num_samples) | |
| return dataset_type, num_samples, None | |
| def _download_and_prepare(self, dataset_dir, scratch_dir, _): | |
| try: | |
| _download_and_extract_archive( | |
| self._GDRIVE_ID, | |
| self._ARCHIVE_NAME, | |
| self._DIR_IN_ARCHIVE, | |
| dataset_dir, | |
| scratch_dir, | |
| ) | |
| except Exception as e: | |
| logger.error("Failed to download or extract archive: %s", e) | |
| raise | |
| logger.info("Parsing dataset metadata") | |
| dataset_type = fot.FiftyOneDataset() | |
| importer = foud.FiftyOneDatasetImporter | |
| num_samples = importer._get_num_samples(dataset_dir) | |
| logger.info("Found %d samples", num_samples) | |
| return dataset_type, num_samples, None |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v0.24.0 #4406 +/- ##
==================================================
Coverage ? 28.72%
==================================================
Files ? 854
Lines ? 105768
Branches ? 1345
==================================================
Hits ? 30385
Misses ? 75383
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
fiftyone/utils/utils3d.py (2)
429-430: Ensure thenormalfield is properly documented.The comment for the
normalfield could be expanded to explain more clearly that it represents the normal vector of the projection plane. This would help users understand its role in the context of orthographic projections.
Line range hint
538-608: Consider adding input validation forpadding.The
paddingparameter should be validated to ensure it falls within the expected range of[0, 1]. This prevents potential runtime errors or unexpected behavior if invalid values are provided.+ if padding is not None and (padding < 0 or padding > 1): + raise ValueError("Padding must be between 0 and 1")
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/utils/utils3d.py (12 hunks)
Additional comments not posted (3)
fiftyone/utils/utils3d.py (3)
440-440: Validate thenormalfield to ensure it always contains three elements.
878-880: Refactor the assignment of the default value forprojection_normal.
925-927: Ensure thenormalfield is correctly populated.
| min_bound = np.asarray(min_bound, dtype=float) | ||
| max_bound = np.asarray(max_bound, dtype=float) | ||
|
|
||
| if padding is not None: | ||
| pad = 0.5 * np.asarray(padding) * (max_bound - min_bound) | ||
| min_bound -= pad | ||
| max_bound += pad | ||
|
|
There was a problem hiding this comment.
Optimize the padding calculation.
The padding calculation can be optimized by directly multiplying the padding ratio by the range, which avoids the need to multiply by 0.5 and then add/subtract the result.
- pad = 0.5 * np.asarray(padding) * (max_bound - min_bound)
- min_bound -= pad
- max_bound += pad
+ pad = np.asarray(padding) * (max_bound - min_bound)
+ min_bound -= 0.5 * pad
+ max_bound += 0.5 * padCommittable 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.
| min_bound = np.asarray(min_bound, dtype=float) | |
| max_bound = np.asarray(max_bound, dtype=float) | |
| if padding is not None: | |
| pad = 0.5 * np.asarray(padding) * (max_bound - min_bound) | |
| min_bound -= pad | |
| max_bound += pad | |
| min_bound = np.asarray(min_bound, dtype=float) | |
| max_bound = np.asarray(max_bound, dtype=float) | |
| if padding is not None: | |
| pad = np.asarray(padding) * (max_bound - min_bound) | |
| min_bound -= 0.5 * pad | |
| max_bound += 0.5 * pad |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
fiftyone/zoo/datasets/base.py (1)
Line range hint
636-636: Avoid using bareexceptstatements.Using bare
exceptstatements can catch unexpected exceptions and obscure programming errors. It's recommended to catch specific exceptions to avoid this issue. Here's a suggested change:- except: + except Exception as e:Also applies to: 643-643, 3360-3360, 3368-3368, 3376-3376, 3383-3383
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- docs/source/user_guide/dataset_zoo/datasets.rst (2 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/source/user_guide/dataset_zoo/datasets.rst
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3360-3360: Do not use bare
except
3368-3368: Do not use bare
except
3376-3376: Do not use bare
except
3383-3383: Do not use bare
except
Additional comments not posted (2)
fiftyone/zoo/datasets/base.py (2)
3182-3204: Introducing theQuickstart3DDatasetclass for handling 3D datasets.This new class provides a structured way to handle 3D datasets, which includes meshes, point clouds, and oriented bounding boxes. The implementation follows the established patterns in the file and integrates well with the existing infrastructure.
3222-3229: Ensure proper error handling in the_download_and_preparemethod.This comment is still valid as the previous review comments on lines 3222-3229 and 3222-3237 have already addressed this issue.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- docs/source/user_guide/using_datasets.rst (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- docs/source/user_guide/using_datasets.rst
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- docs/source/user_guide/using_datasets.rst (3 hunks)
- fiftyone/utils/utils3d.py (12 hunks)
Files skipped from review as they are similar to previous changes (2)
- docs/source/user_guide/using_datasets.rst
- fiftyone/utils/utils3d.py
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
fiftyone/zoo/datasets/base.py (1)
Line range hint
636-636: Avoid using bareexceptstatements.- except: + except Exception as e:Also applies to: 643-643, 3360-3360, 3368-3368, 3376-3376, 3383-3383
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (4)
docs/source/images/datasets/quickstart-3d.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/datasets/quickstart-groups.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/datasets/quickstart-video.gifis excluded by!**/*.gif,!**/*.gifdocs/source/images/datasets/quickstart.gifis excluded by!**/*.gif,!**/*.gif
Files selected for processing (5)
- docs/source/user_guide/dataset_zoo/datasets.rst (2 hunks)
- docs/source/user_guide/using_datasets.rst (9 hunks)
- fiftyone/core/threed/scene_3d.py (6 hunks)
- fiftyone/utils/utils3d.py (13 hunks)
- fiftyone/zoo/datasets/base.py (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- docs/source/user_guide/dataset_zoo/datasets.rst
- docs/source/user_guide/using_datasets.rst
- fiftyone/utils/utils3d.py
Additional Context Used
Ruff (6)
fiftyone/zoo/datasets/base.py (6)
636-636: Do not use bare
except
643-643: Do not use bare
except
3360-3360: Do not use bare
except
3368-3368: Do not use bare
except
3376-3376: Do not use bare
except
3383-3383: Do not use bare
except
Additional comments not posted (7)
fiftyone/core/threed/scene_3d.py (4)
116-138: The example usage provided in the comments is clear and demonstrates how to use theSceneclass effectively.
253-254: The methodtraverseis well-documented, and the return type is clearly specified.
316-321: The methodget_asset_pathsis well-implemented. It efficiently gathers all asset paths from the scene, including those from the background. The use ofitertools.chain.from_iterablefor flattening lists is a good choice for performance.
386-393: The methodfrom_fo3dcorrectly checks the file extension before proceeding with loading, which is a good practice for ensuring that the file format is as expected.fiftyone/zoo/datasets/base.py (3)
3222-3237: Ensure proper error handling in the_download_and_preparemethod.
3222-3237: Specify exception types in try-except blocks to avoid suppressing unexpected errors.
3182-3237: The implementation ofQuickstart3DDatasetlooks correct and follows the established patterns for dataset classes in FiftyOne. The documentation is clear and provides all necessary details for users to understand and use the dataset effectively.
Quickstart 3D dataset
200 samples (5 x 40) of a 3D mesh dataset. Point clouds are generated by oversampling then condensing via poisson disk sampling in open3d. Bounding boxes are oriented bounding boxes from open3D.
Orthographic projection of the generated PCD is also included.
Summary by CodeRabbit
New Features
DetectionLabelinterface to include convex hull rendering.Documentation
Bug Fixes
Tests
Refactor