3D export fixes and tests #4368
Conversation
WalkthroughThe recent updates focus on enhancing asset path management within the Changes
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 (
|
Ok I am seeing that this exists, sort of, in Teams when a cloud path is involved. I am unable to take this refactor on right now so perhaps I'll have to leave it alone for now. |
960d88a to
1a8b9ba
Compare
|
@coderabbitai review |
Actions PerformedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (9)
fiftyone/utils/data/exporters.py (9)
Line range hint
541-541: Avoid using bareexceptstatements.- except: + except Exception as e: + # Handle or log the exception e appropriately
Line range hint
571-571: Avoid using bareexceptstatements.- except: + except Exception as e: + # Handle or log the exception e appropriately
Line range hint
614-614: Avoid using bareexceptstatements.- except: + except Exception as e: + # Handle or log the exception e appropriately
Line range hint
649-649: Avoid using bareexceptstatements.- except: + except Exception as e: + # Handle or log the exception e appropriately
Line range hint
904-904: Avoid using bareexceptstatements.- except: + except Exception as e: + # Handle or log the exception e appropriately
Line range hint
3821-3821: Avoid inequality comparisons toFalse; useif include_confidence:for truth checks.- if include_confidence == True: + if include_confidence:
Line range hint
3821-3821: Avoid inequality comparisons toFalse; useif include_attributes:for truth checks.- if include_attributes == True: + if include_attributes:
Line range hint
3947-3947: Avoid equality comparisons toTrue; useif include_confidence:for truth checks.- if include_confidence == True: + if include_confidence:
Line range hint
3952-3952: Avoid equality comparisons toTrue; useif include_attributes:for truth checks.- if include_attributes == True: + if include_attributes:
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- fiftyone/core/threed/scene_3d.py (1 hunks)
- fiftyone/utils/data/exporters.py (3 hunks)
- tests/unittests/import_export_tests.py (2 hunks)
Additional Context Used
Ruff (15)
fiftyone/utils/data/exporters.py (9)
541-541: Do not use bare
except
571-571: Do not use bare
except
614-614: Do not use bare
except
649-649: Do not use bare
except
904-904: Do not use bare
except
3821-3821: Avoid inequality comparisons to
False; useif include_confidence:for truth checks
3821-3821: Avoid inequality comparisons to
False; useif include_attributes:for truth checks
3947-3947: Avoid equality comparisons to
True; useif include_confidence:for truth checks
3952-3952: Avoid equality comparisons to
True; useif include_attributes:for truth checkstests/unittests/import_export_tests.py (6)
2134-2134:
pydicomimported but unused
3572-3572: Local variable
img_filepathis assigned to but never used
3589-3589: Local variable
img_filepathis assigned to but never used
3607-3607: Local variable
img_filepathis assigned to but never used
3636-3636: Local variable
img_filepathis assigned to but never used
3706-3706: Local variable
vid_filepathis assigned to but never used
Additional comments not posted (9)
tests/unittests/import_export_tests.py (9)
4653-4656: The classThreeDMediaTestsis well-documented, providing a clear purpose for the tests.
4658-4679: The method_build_flat_relativeeffectively sets up a test environment for a flat, relative-addressed 3D scene. It correctly handles file operations and temporary directories.
4681-4711: The method_build_flat_absoluteis well-implemented for setting up a flat, absolute-addressed 3D scene. It ensures that paths are absolute and correctly writes dummy data to files.
4713-4772: The method_build_nested_relativecorrectly sets up a more complex nested directory structure for testing 3D scenes with relative paths. It handles multiple scenes and ensures that asset paths are relative.
4774-4780: The method_assert_scene_contentprovides a robust way to verify the integrity of 3D scenes by checking file contents against expected values. This is crucial for ensuring that export functionalities are working as expected.
4783-4806: The test methodtest_flat_relativeeffectively verifies the export functionality for a flat, relative-addressed 3D scene. It includes steps for exporting the scene and then using_assert_scene_contentto verify the integrity of the exported files.
4808-4837: The methodtest_flat_absolutecorrectly tests the export functionality for a flat, absolute-addressed 3D scene. It ensures that the scene is exported correctly and verifies the content post-export.
4840-4914: The methodtest_relative_nested_flatteneffectively tests the export functionality for a nested, relative-addressed 3D scene, ensuring that the directory structure is flattened during export and that file names are correctly handled to avoid conflicts.
4916-4956: The methodtest_relative_nested_maintainthoroughly tests the export functionality to ensure that a nested directory structure is maintained during the export of a 3D scene. It verifies that relative paths in the.fo3dfiles remain unchanged, which is crucial for the integrity of the scene.
| def update_asset_paths(self, asset_rewrite_paths: dict): | ||
| """Update asset paths in this scene according to an input dict mapping. | ||
|
|
||
| Asset path is unchanged if it does not exist in ``asset_rewrite_paths`` | ||
|
|
||
| Args: | ||
| asset_rewrite_paths: ``dict`` mapping asset path to new asset path | ||
|
|
||
| Returns: | ||
| ``True`` if the scene was modified. | ||
| """ | ||
| scene_modified = False | ||
| for node in self.traverse(): | ||
| for path_attribute in node._asset_path_fields: | ||
| asset_path = getattr(node, path_attribute, None) | ||
| new_asset_path = asset_rewrite_paths.get(asset_path) | ||
|
|
||
| if asset_path is not None and asset_path != new_asset_path: | ||
| setattr(node, path_attribute, new_asset_path) | ||
| scene_modified = True | ||
|
|
||
| # modify scene background paths, if any | ||
| if self.background is not None: | ||
| if self.background.image is not None: | ||
| new_asset_path = asset_rewrite_paths.get(self.background.image) | ||
| if new_asset_path != self.background.image: | ||
| self.background.image = new_asset_path | ||
| scene_modified = True | ||
|
|
||
| if self.background.cube is not None: | ||
| new_cube = [ | ||
| asset_rewrite_paths.get(face) | ||
| for face in self.background.cube | ||
| ] | ||
| if new_cube != self.background.cube: | ||
| self.background.cube = new_cube | ||
| scene_modified = True | ||
|
|
||
| return scene_modified |
There was a problem hiding this comment.
Consider enhancing traceability and robustness in asset path updates.
- It would be beneficial to add logging at key points in this method to trace the changes being made to asset paths, which can be crucial for debugging and verifying the behavior in production.
- Additionally, consider handling the scenario where
new_asset_pathisNone. Currently, ifasset_rewrite_paths.get(asset_path)returnsNone, it will replace a valid path withNone, which might not be the intended behavior. You might want to add a condition to check ifnew_asset_pathis notNonebefore setting it.
+ if asset_path is not None and new_asset_path is not None and asset_path != new_asset_path: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.
| def update_asset_paths(self, asset_rewrite_paths: dict): | |
| """Update asset paths in this scene according to an input dict mapping. | |
| Asset path is unchanged if it does not exist in ``asset_rewrite_paths`` | |
| Args: | |
| asset_rewrite_paths: ``dict`` mapping asset path to new asset path | |
| Returns: | |
| ``True`` if the scene was modified. | |
| """ | |
| scene_modified = False | |
| for node in self.traverse(): | |
| for path_attribute in node._asset_path_fields: | |
| asset_path = getattr(node, path_attribute, None) | |
| new_asset_path = asset_rewrite_paths.get(asset_path) | |
| if asset_path is not None and asset_path != new_asset_path: | |
| setattr(node, path_attribute, new_asset_path) | |
| scene_modified = True | |
| # modify scene background paths, if any | |
| if self.background is not None: | |
| if self.background.image is not None: | |
| new_asset_path = asset_rewrite_paths.get(self.background.image) | |
| if new_asset_path != self.background.image: | |
| self.background.image = new_asset_path | |
| scene_modified = True | |
| if self.background.cube is not None: | |
| new_cube = [ | |
| asset_rewrite_paths.get(face) | |
| for face in self.background.cube | |
| ] | |
| if new_cube != self.background.cube: | |
| self.background.cube = new_cube | |
| scene_modified = True | |
| return scene_modified | |
| def update_asset_paths(self, asset_rewrite_paths: dict): | |
| """Update asset paths in this scene according to an input dict mapping. | |
| Asset path is unchanged if it does not exist in ``asset_rewrite_paths`` | |
| Args: | |
| asset_rewrite_paths: ``dict`` mapping asset path to new asset path | |
| Returns: | |
| ``True`` if the scene was modified. | |
| """ | |
| scene_modified = False | |
| for node in self.traverse(): | |
| for path_attribute in node._asset_path_fields: | |
| asset_path = getattr(node, path_attribute, None) | |
| new_asset_path = asset_rewrite_paths.get(asset_path) | |
| if asset_path is not None and new_asset_path is not None and asset_path != new_asset_path: | |
| setattr(node, path_attribute, new_asset_path) | |
| scene_modified = True | |
| # modify scene background paths, if any | |
| if self.background is not None: | |
| if self.background.image is not None: | |
| new_asset_path = asset_rewrite_paths.get(self.background.image) | |
| if new_asset_path != self.background.image: | |
| self.background.image = new_asset_path | |
| scene_modified = True | |
| if self.background.cube is not None: | |
| new_cube = [ | |
| asset_rewrite_paths.get(face) | |
| for face in self.background.cube | |
| ] | |
| if new_cube != self.background.cube: | |
| self.background.cube = new_cube | |
| scene_modified = True | |
| return scene_modified |
* WIP 3D export fixes and tests * print * errant copypasta * move rewrite asset logic to scene_3d * cleanup 3d import/export tests a bit
Checkpointing my work in case I don't get to finish it.
rel_dir) causes a file to be renamed from, say,pcd.pcdtopcd-2.pcdThe tests are absolutely horrendous on the eyes, I'm so sorry!! They could be refactored to something nicer but didn't have time right now.
What I would like to do if I have time:
asset_dirfos.resolve()and a cloud friendlyos.path.relpath()if it doesn't already exist. Actually the latter probably just works but we would want to double check.fos.upload_media()anddownload_media()in Teams so we don't have the same fragile code implemented twice. With the side benefit that export should be faster.Summary by CodeRabbit