Release/v0.25.1 #4774
Release/v0.25.1 #4774
Conversation
* modal filter + tagging fix, e2e sketch * e2e with screenshot assertions * lint unused * add back sample_ids * event tweak
* handle keypoint and keypoints * other tests * lint
Plugin docs tweak
Cherry-picking #4742 for release
* lint graphQLSyncFragmentAtomFamily * rm unused exposed function, add on set for grid sidebar * fix sidebar persitence * add expansion inheritance
Fix bug with group_by(..., match_expr=)
* clear records on page refresh * refactor as independent hook, add hook test * rm useRef * fixing selections with partial index records * add range tests * relay cleanup tweak * typos
Cherry pick #4715
Release notes for v0.25.1
WalkthroughThe changes encompass various enhancements across multiple files, including modifications to component properties, the introduction of new hooks and tests, and adjustments to state management. Key updates involve the restructuring of type definitions, the implementation of new testing strategies for hooks and components, and refinements in the handling of user interactions and data processing within the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModalSidebarPom
participant Grid
participant Records
User->>ModalSidebarPom: Interact with sidebar
ModalSidebarPom->>Grid: Apply filters
Grid->>Records: Update records based on filters
Records-->>Grid: Return updated records
Grid-->>User: Display updated grid
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: 5
Outside diff range, codebase verification and nitpick comments (2)
e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1)
16-21: Approve the implementation ofapplyFilter.The
applyFiltermethod correctly uses thelocatorto interact with the UI elements based on the provided label. The use offorce: truein the click method is noted, which might be necessary for elements that are not easily interactable due to styling or visibility issues.Consider adding a comment in the code to explain the use of
force: trueto aid future maintainers or testers in understanding the choice.docs/source/release-notes.rst (1)
Line range hint
1-1: Reminder: Add tests.The TODO comment indicates that tests are missing.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (5)
app/packages/app/src/pages/datasets/__generated__/DatasetPageQuery.graphql.tsis excluded by!**/__generated__/**app/packages/relay/src/fragments/__generated__/sidebarGroupsFragment.graphql.tsis excluded by!**/__generated__/**app/packages/relay/src/queries/__generated__/datasetQuery.graphql.tsis excluded by!**/__generated__/**e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts-snapshots/labels-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts-snapshots/labels-chromium-linux.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (31)
- app/packages/components/src/components/PillButton/PillButton.tsx (1 hunks)
- app/packages/core/src/components/Grid/Grid.tsx (5 hunks)
- app/packages/core/src/components/Grid/useRecords.test.ts (1 hunks)
- app/packages/core/src/components/Grid/useRecords.ts (1 hunks)
- app/packages/core/src/components/Grid/useSelect.ts (1 hunks)
- app/packages/core/src/components/Grid/useSelectSample.test.ts (1 hunks)
- app/packages/core/src/components/Grid/useSelectSample.ts (4 hunks)
- app/packages/core/src/components/Grid/useSpotlightPager.ts (5 hunks)
- app/packages/core/src/components/Modal/Sample.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3 hunks)
- app/packages/core/src/plugins/SchemaIO/components/TabsView.tsx (2 hunks)
- app/packages/relay/src/fragments/sidebarGroupsFragment.ts (1 hunks)
- app/packages/relay/src/graphQLSyncFragmentAtomFamily.ts (2 hunks)
- app/packages/state/src/hooks/useSetModalState.ts (2 hunks)
- app/packages/state/src/recoil/pathFilters/index.test.ts (1 hunks)
- app/packages/state/src/recoil/pathFilters/index.ts (5 hunks)
- app/packages/state/src/recoil/sidebar.ts (2 hunks)
- app/packages/state/src/recoil/sidebarExpanded.ts (2 hunks)
- docs/source/plugins/developing_plugins.rst (1 hunks)
- docs/source/release-notes.rst (1 hunks)
- e2e-pw/src/oss/poms/modal/modal-sidebar.ts (2 hunks)
- e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts (1 hunks)
- e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (7 hunks)
- fiftyone/core/stages.py (1 hunks)
- fiftyone/operators/delegated.py (1 hunks)
- fiftyone/operators/panel.py (2 hunks)
- fiftyone/server/routes/tag.py (1 hunks)
- fiftyone/utils/ultralytics.py (1 hunks)
- fiftyone/utils/yolo.py (1 hunks)
- tests/unittests/group_tests.py (1 hunks)
- tests/unittests/panels/panel_tests.py (3 hunks)
Files skipped from review due to trivial changes (3)
- app/packages/components/src/components/PillButton/PillButton.tsx
- app/packages/core/src/components/Grid/useSelect.ts
- docs/source/plugins/developing_plugins.rst
Additional context used
Path-based instructions (19)
app/packages/core/src/components/Grid/useRecords.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/relay/src/fragments/sidebarGroupsFragment.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/core/src/components/Grid/useRecords.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/core/src/components/Grid/useSelectSample.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/state/src/recoil/pathFilters/index.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/state/src/recoil/sidebarExpanded.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/core/src/plugins/SchemaIO/components/TabsView.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.app/packages/core/src/components/Modal/Sample.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.e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.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/state/src/hooks/useSetModalState.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.e2e-pw/src/oss/poms/modal/modal-sidebar.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/core/src/components/Grid/useSelectSample.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/core/src/components/Grid/useSpotlightPager.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.e2e-pw/src/oss/specs/smoke-tests/tagger.spec.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/core/src/components/Grid/Grid.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.app/packages/relay/src/graphQLSyncFragmentAtomFamily.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/state/src/recoil/pathFilters/index.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/core/src/components/Sidebar/Entries/GroupEntries.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.app/packages/state/src/recoil/sidebar.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 (60)
app/packages/relay/src/fragments/sidebarGroupsFragment.ts (1)
5-5: Verify the handling ofdatasetIdacross the application.The change from
nametodatasetIdin the GraphQL fragment could significantly impact how data is processed and displayed in the application. Ensure that all components and functionalities that rely on this fragment are updated to accommodate the new field.Run the following script to verify the handling of
datasetIdacross the application:app/packages/core/src/components/Grid/useRecords.test.ts (1)
5-23: Well-structured tests foruseRecordshook.The tests are comprehensive and effectively check the hook's behavior when the
clearstring changes. The use ofrenderHookandrerenderis appropriate and follows best practices for testing React hooks.app/packages/core/src/components/Grid/useSelectSample.test.ts (1)
4-29: Comprehensive and well-structured tests for range operations.The tests for
addRangeandremoveRangefunctions are well-structured and seem to cover the basic scenarios effectively. UsingSetandMapis appropriate for the operations being tested. It's good practice to include tests for edge cases, such as adding or removing empty ranges or handling non-existent elements in the collections.app/packages/state/src/recoil/pathFilters/index.test.ts (1)
10-26: Effective testing of keypoint filters with clear differentiation of behavior.The tests effectively differentiate the behavior of the
keypointFilterbased on the field type, which is crucial for ensuring that the filter logic is robust and behaves as expected across different scenarios. Using constants for field types helps avoid errors and improves maintainability. Consider adding tests for more complex scenarios or invalid inputs to further ensure the filter's robustness.app/packages/state/src/recoil/sidebarExpanded.ts (1)
8-8: Key renaming in Recoil state management entities enhances clarity.The changes to the keys of
sidebarExpandedStoreandsidebarExpandedimprove semantic clarity and help prevent potential naming conflicts. It's important to verify that these changes are consistently applied across the entire application to avoid issues with state management. Consider using a global search or refactoring tool to ensure all references to these keys are updated.Run the following script to verify the consistent application of the new keys:
Also applies to: 22-22
Verification successful
Key renaming in Recoil state management entities is consistently applied.
The keys
sidebarExpandedStoreandsidebarExpandedhave been consistently updated across the application, as evidenced by their presence in various files related to state management and components. This confirms that the renaming has been correctly implemented without leaving any outdated references.
- Files where the keys are used include
filters.ts,sidebarExpanded.ts,useSetModalState.ts,index.ts,lightning.ts, and several component files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new keys are consistently used across the application. # Test: Search for the old and new keys. Expect: Only occurrences of the new keys. rg --type typescript -A 5 $'sidebarExpandedStore|sidebarExpanded'Length of output: 101
Script:
#!/bin/bash # Description: Verify that the new keys are consistently used across the application. # Test: Search for the old and new keys. Expect: Only occurrences of the new keys. rg --type ts -A 5 $'sidebarExpandedStore|sidebarExpanded'Length of output: 7923
app/packages/core/src/plugins/SchemaIO/components/TabsView.tsx (2)
7-7: Correct use ofuseKeyhook.The import and usage of the
useKeyhook are correctly implemented to manage user interaction states effectively.Also applies to: 14-14
35-38: Enhanced user interaction handling inTabsView.The modifications to the
onChangehandler to includesetUserChangedenhance the responsiveness of the component to user actions. This is a good practice for state management in interactive components.app/packages/core/src/components/Modal/Sample.tsx (1)
8-8: Proper integration ofuseKeyDownfor enhanced keyboard interaction.The import and usage of the
useKeyDownhook within theSampleWrappercomponent are correctly implemented to handle keyboard events, enhancing the component's interactivity and accessibility.Also applies to: 29-32
e2e-pw/src/oss/specs/smoke-tests/quickstart.spec.ts (1)
89-94: Well-implemented test case for sidebar persistence.The new test case "sidebar persistence" is well-structured and effectively verifies the sidebar's state after interactions, ensuring robustness in the testing framework.
fiftyone/server/routes/tag.py (1)
72-80: Review the changes in filter handling and modal response logic.The method now uses an empty dictionary for filters, which could potentially ignore user-specified filters and affect the functionality. Ensure that this change aligns with the intended behavior and does not inadvertently filter out necessary data.
Additionally, the handling of the
modalflag to determine the response format needs careful review to ensure it meets the user interface expectations.Consider adding comments or documentation to clarify the reasons behind these changes and their expected impact on the application.
app/packages/state/src/hooks/useSetModalState.ts (2)
13-13: Approve the new import for sidebar state management.The import of
sidebarExpandedAtomsis correctly placed and used within the function to enhance the sidebar's dynamic state management. This addition aligns with the described enhancements in the PR summary.
66-68: Approve the updated logic for managing the sidebar's expanded state.The addition of conditions for managing the sidebar's expanded state at lines 66-68 enhances the dynamic interactions within the user interface. This update appears to integrate well with the existing state management logic.
Consider adding unit tests to ensure that these new conditions integrate seamlessly with the existing functionality and do not introduce regressions.
e2e-pw/src/oss/poms/modal/modal-sidebar.ts (4)
23-27: Approve the implementation ofapplySearch.The
applySearchmethod is well-implemented, correctly simulating a search operation by filling the input and pressing the "Enter" key. This method aligns with user expectations for search interactions within the sidebar.
29-31: Approve the implementation ofclearGroupFilters.The
clearGroupFiltersmethod is efficiently implemented, using thelocatorto interact with the clear filters button for a specific group. This functionality is essential for allowing users to reset filters easily within the sidebar.
33-37: Approve the implementation ofclickFieldDropdown.The
clickFieldDropdownmethod is well-implemented, using thelocatorto interact with the dropdown arrow for specified fields. This method enhances the interactivity of the sidebar by allowing users to access additional options or settings related to different fields.
74-76: Approve the implementation oftoggleLabelCheckbox.The
toggleLabelCheckboxmethod is efficiently implemented, allowing for quick toggling of checkboxes for specified fields. This functionality is crucial for testing different configurations rapidly within the sidebar.app/packages/core/src/components/Grid/useSelectSample.ts (4)
1-9: Review of Imports and Initial SetupThe imports and initial setup look appropriate for the functionality described. The use of Recoil for state management and the structured import of types and utilities suggest a well-organized approach to state handling and component interaction.
44-53: Utility Functiongetfor Record RetrievalThe
getfunction provides a robust way to retrieve record indices with added error handling. This is a significant improvement in terms of code reliability and maintainability. Throwing an error when a record is not found (line 52) ensures that the function's consumers handle missing data proactively.
Line range hint
55-99: Refined Logic inremoveRangeFunctionThe
removeRangefunction mirrors the enhancements seen inaddRange, with improved clarity and error handling. The logic to determine the range of indices to remove is well thought out, especially the handling of edge cases where the index might be at the start or end of the range.The loop for adding back items not present in the index records (lines 93-97) is a good safeguard against data inconsistency.
Line range hint
102-143: Modular Default Export FunctionThe default export function now accepts
recordsas a parameter, enhancing its modularity and reusability. The use of Recoil callbacks and the structured handling of selection logic based on user interactions (shift key usage, sample selection/deselection) are well-implemented.The integration of
addRangeandremoveRangewithin user interaction handling (lines 121-124) demonstrates a good understanding of functional programming and state management in React applications.app/packages/core/src/components/Grid/useSpotlightPager.ts (2)
Line range hint
17-60: Updated Function Signature and Logic inuseSpotlightPagerThe introduction of the
recordsparameter in theuseSpotlightPagerfunction (line 59) is a significant change that enhances the function's ability to interact with external state. This change allows for more flexible and maintainable code by decoupling the pager logic from the internal state management previously handled withuseRef.The use of
recordsdirectly in the function body (line 97) to process pagination data ensures that the function can adapt to changes in the external state more dynamically.
Line range hint
70-128: Refactoring and Error Handling EnhancementsThe refactoring to use a
keysuseReffor managing record invalidation keys (line 70) is a thoughtful addition that separates concerns more clearly than before. The handling of asynchronous operations within theuseRecoilCallback(lines 70-99) is robust, with proper error handling and subscription management.The cleanup logic in the
useEffecthook (lines 124-128) ensures that the component cleans up after itself, preventing potential memory leaks and ensuring that the UI remains consistent with the underlying data state.e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (2)
4-26: Integration ofModalPomand Refactoring of Test SetupThe introduction of
ModalPom(line 12) and the restructuring of the test setup to accommodate this change are significant improvements. The removal ofgridActionsRowand its integration into thegridobject streamline the test context and reduce redundancy.The asynchronous setup functions for
grid,modal,sidebar, andtagger(lines 16-26) are well-structured, ensuring that each component is properly initialized before tests run.
Line range hint
42-129: Updated Test Cases Reflecting New Modal FunctionalitiesThe test cases have been updated to reflect the integration of the
ModalPom. The new test case "In modal, I can add a label tag to a filtered sample" (lines 102-129) is a valuable addition that expands the test coverage to include modal interactions.The use of promises to handle asynchronous events (lines 113-118) and the clear structuring of actions and expectations within each test case ensure that the tests are both readable and maintainable.
app/packages/core/src/components/Grid/Grid.tsx (2)
4-6: Approved: Import ReorganizationThe reorganization of imports enhances clarity and maintainability without affecting functionality.
78-78: Verify: UpdatedselectSample.currentFunction CallThe update to the
selectSample.currentfunction call now passing only thedetailparameter simplifies the logic. Verify that this change aligns with the intended behavior and that all dependent functionalities are updated accordingly.app/packages/relay/src/graphQLSyncFragmentAtomFamily.ts (1)
59-60: Approved: Comment Reformatting and Enhanced Property AccessThe reformatting of comments improves readability and maintainability. The introduction of optional chaining (
fragmentOptions?.keys[i]) enhances the robustness of the code by safely accessing properties, which is a best practice in handling potentialnullorundefinedvalues.Also applies to: 97-97
app/packages/state/src/recoil/pathFilters/index.ts (1)
9-10: Approved: Introduction of Keypoint Handling EnhancementsThe introduction of new constants (
KEYPOINT_FIELD,KEYPOINTS_FIELD) and thekeypointFilterfunction significantly enhances the filtering capabilities of the module. These changes are well-implemented and improve the clarity and functionality of the filtering process.Also applies to: 35-35, 172-188
tests/unittests/panels/panel_tests.py (7)
186-192: Test for setting state with a dictionary is correctly implemented.The test verifies both the state update and the method call, ensuring the functionality behaves as expected.
195-201: Test for setting state with a nested key path is correctly implemented.The test ensures that nested paths are correctly updated and the corresponding method call is verified, which is crucial for ensuring the robustness of state management.
204-210: Test for setting state with a nested key path using a dictionary is correctly implemented.This test verifies the correct handling of nested paths using a dictionary format, ensuring that the state management is robust and handles complex scenarios effectively.
213-217: Test for enforcing the write-only property on data attribute is correctly implemented.This test correctly checks that accessing the
dataattribute raises aWriteOnlyError, which is essential for maintaining the integrity of the write-only property.
234-240: Test for setting data with a dictionary is correctly implemented.This test verifies that data can be correctly updated using a dictionary, and it checks that the appropriate method call is made, ensuring the functionality behaves as expected.
243-249: Test for setting data with a nested key path is correctly implemented.This test ensures that nested paths in data are correctly updated and the corresponding method call is verified, which is crucial for ensuring the robustness of data management.
252-258: Test for setting data with a nested key path using a dictionary is correctly implemented.This test verifies the correct handling of nested paths using a dictionary format, ensuring that the data management is robust and handles complex scenarios effectively.
fiftyone/operators/delegated.py (1)
408-409: Improved Error Handling: Conditional Check onoutputsThe addition of a conditional check before invoking
to_json()onoutputsenhances the robustness of the_execute_operatormethod by preventing potentialAttributeErrorexceptions. This is a good practice for handlingNonevalues in Python.fiftyone/utils/ultralytics.py (1)
61-61: Code Simplification and Correction in_extract_track_idsThe modification to use
result.boxes.conf.size(0)directly for generating a list ofNonevalues simplifies the code and corrects the logic. This change enhances clarity and ensures the correct size is used for the list.app/packages/state/src/recoil/sidebar.ts (2)
217-217: Refined Expansion Logic inresolveGroupsThe update to check explicitly for a boolean value in the
expandedobject enhances the robustness of the group expansion logic. This prevents unintended behavior from non-boolean truthy or falsy values, aligning with best practices for type safety in JavaScript.
319-366: Streamlined Export Structure insidebarGroupsDefinitionThe simplification of the
sidebarGroupsDefinitionexport by directly returning thegraphQLSyncFragmentAtomFamilyfunction and refining the data comparison logic in thereadfunction enhances clarity and correctness. The focus on dataset ID for state updates is a more reliable approach than using names, which improves the robustness of data handling.docs/source/release-notes.rst (1)
3-4: LGTM!The code changes are approved.
fiftyone/core/stages.py (15)
Line range hint
1-1: ClassConcatReview:The
Concatclass is implemented correctly and uses appropriate MongoDB operations to merge collections. The validation logic invalidateis robust, ensuring that only compatible collections are concatenated.
Line range hint
1-1: ClassExcludeReview:The
Excludeclass is implemented correctly and uses appropriate MongoDB operations to exclude samples based on IDs. The validation logic invalidateensures that only valid IDs are used for exclusion.
Line range hint
1-1: ClassExcludeByReview:The
ExcludeByclass is implemented correctly and uses appropriate MongoDB operations to exclude samples based on field values. The validation logic invalidateensures that the field exists in the dataset before exclusion.
Line range hint
1-1: ClassExcludeFieldsReview:The
ExcludeFieldsclass is implemented correctly and uses appropriate MongoDB operations to exclude specified fields from samples. The validation logic invalidateensures that the fields exist in the dataset before exclusion.
Line range hint
1-1: ClassExcludeFramesReview:The
ExcludeFramesclass is implemented correctly and uses appropriate MongoDB operations to exclude specified frames from video samples. The validation logic invalidateensures that the collection contains videos before exclusion.
Line range hint
1-1: ClassExcludeGroupsReview:The
ExcludeGroupsclass is implemented correctly and uses appropriate MongoDB operations to exclude specified groups from samples. The validation logic invalidateensures that the collection contains groups before exclusion.
Line range hint
1-1: ClassExcludeLabelsReview:The
ExcludeLabelsclass is implemented correctly and uses appropriate MongoDB operations to exclude specified labels from samples based on multiple criteria. The validation logic invalidateensures that the labels exist in the dataset before exclusion.
Line range hint
1-1: ClassExistsReview:The
Existsclass is implemented correctly and uses appropriate MongoDB operations to filter samples based on the existence of a specified field. The validation logic invalidateensures that the field exists in the dataset before filtering.
Line range hint
1-1: ClassFilterFieldReview:The
FilterFieldclass is implemented correctly and uses appropriate MongoDB operations to filter the values of a specified field in samples based on multiple criteria. The validation logic invalidateensures that the field exists in the dataset before filtering.
Line range hint
1-1: ClassFilterLabelsReview:The
FilterLabelsclass is implemented correctly and uses appropriate MongoDB operations to filter labels in a specified field of samples based on multiple criteria. The validation logic invalidateensures that the field exists in the dataset before filtering.
Line range hint
1-1: ClassFilterKeypointsReview:The
FilterKeypointsclass is implemented correctly and uses appropriate MongoDB operations to filter keypoints in a specified field of samples based on multiple criteria. The validation logic invalidateensures that the field exists in the dataset before filtering.
Line range hint
1-1: ClassFlattenReview:The
Flattenclass is implemented correctly and uses appropriate MongoDB operations to flatten a dynamically grouped collection into a non-grouped collection. The validation logic invalidateensures that the collection is dynamically grouped before flattening.
Line range hint
1-1: ClassGeoNearReview:The
GeoNearclass is implemented correctly and uses appropriate MongoDB operations to sort samples based on their proximity to a specified geolocation. The validation logic invalidateensures that the location field is specified and creates a 2dsphere index if necessary.
Line range hint
1-1: ClassGeoWithinReview:The
GeoWithinclass is implemented correctly and uses appropriate MongoDB operations to filter samples based on whether their geolocation falls within a specified boundary. The validation logic invalidateensures that the location field is specified and creates a 2dsphere index if necessary.
Line range hint
1-1: ClassGroupByReview:The
GroupByclass is implemented correctly and uses appropriate MongoDB operations to group samples based on a specified field or expression. The validation logic invalidateensures that the collection is not already dynamically grouped before grouping.fiftyone/operators/panel.py (2)
249-250: Simplified data setting inPanelRefData.The changes to directly use
super().set(d)andself._ctx.ops.patch_panel_data(d)are approved as they simplify the code and potentially enhance performance. Ensure that these changes integrate seamlessly with the superclass'ssetmethod and thepatch_panel_datamethod.Run the following script to verify the integration:
Verification successful
Integration of
super().set(d)inPanelRefDatais correct.The
super().set(d)call inPanelRefDatacorrectly invokes thesetmethod from its superclassPanelRefBase, ensuring proper integration. The usage ofpatch_panel_datais confirmed, but its implementation details are not fully visible. Consider manually verifying thepatch_panel_datamethod if further assurance is needed.
PanelRefDatacorrectly inherits and overrides thesetmethod fromPanelRefBase.patch_panel_datais used inPanelRefData, but further details are not visible.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the `set` method with the superclass and `patch_panel_data`. # Test: Search for the superclass `set` method and `patch_panel_data` method usage. Expect: Proper handling of the dictionary `d`. rg --type python -A 5 $'super().set' fiftyone/operators/panel.py rg --type python -A 5 $'patch_panel_data' fiftyone/operators/panel.pyLength of output: 196
Script:
#!/bin/bash # Description: Verify the integration of the `set` method with the superclass and `patch_panel_data`. # Test: Search for the superclass `set` method and `patch_panel_data` method usage. Expect: Proper handling of the dictionary `d`. rg --type py -A 5 $'super().set' fiftyone/operators/panel.py rg --type py -A 5 $'patch_panel_data' fiftyone/operators/panel.pyLength of output: 286
Script:
#!/bin/bash # Description: Verify the class hierarchy and the usage of the `set` method in the superclass. # Search for the class definition containing the `set` method. rg --type py 'class ' fiftyone/operators/panel.py -A 10 # Search for the `set` method definition in the superclass. rg --type py 'def set' fiftyone/operators/panel.py -A 5Length of output: 3271
215-216: Simplified state setting inPanelRefState.The changes to directly use
super().set(d)andself._ctx.ops.patch_panel_state(d)are approved as they simplify the code and potentially enhance performance. However, ensure that these changes integrate seamlessly with the superclass'ssetmethod and thepatch_panel_statemethod.Run the following script to verify the integration:
app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3)
35-36: Enhanced testability withdataCyproperty inPillEntry.The addition of the optional
dataCyproperty to thePillEntrytype is approved as it enhances testability and accessibility by providing a unique identifier for each pill entry.
47-52: Improved flexibility and testability inPillscomponent.The update to destructure and use the
dataCyproperty in thePillscomponent is approved. This change enhances the component's flexibility and usability in testing scenarios by allowingdataCyto be passed as a prop to thePillButtoncomponent.
311-332: Enhanced clarity and specificity withdataCyusage inPathGroupEntry.The specific assignment of
dataCyvalues based on the context in thePathGroupEntrycomponent is approved. This modification improves the clarity and specificity of the identifiers used for testing and interaction tracking, enhancing the overall maintainability of the code.
| export default (clear: string) => { | ||
| return useMemo(() => { | ||
| clear; | ||
| return new Map<string, number>(); | ||
| }, [clear]); |
There was a problem hiding this comment.
Clarify the usage of the clear parameter in useMemo.
The clear parameter is included in the dependency array but does not seem to affect the memoized value directly. This could potentially lead to bugs or confusion about the hook's behavior. Consider either using the clear parameter to affect the state or clarify its purpose if it's intended to trigger re-computation without directly influencing the result.
| export const addRange = ( | ||
| index: number, | ||
| selected: Set<string>, | ||
| records: Records | ||
| ) => { | ||
| // filter selections without an index record | ||
| const items = [...selected].filter((i) => records.has(i)); | ||
| const reverse = Object.fromEntries( | ||
| Array.from(records.current.entries()).map(([k, v]) => [v, k]) | ||
| ); | ||
|
|
||
| const min = argMin( | ||
| items.map((id) => Math.abs(records.current.get(id) - index)) | ||
| Array.from(records.entries()).map(([k, v]) => [v, k]) | ||
| ); | ||
| const min = argMin(items.map((id) => Math.abs(get(records, id) - index))); | ||
|
|
||
| const close = records.current.get(items[min]); | ||
| const close = get(records, items[min]); | ||
|
|
||
| const [start, end] = index < close ? [index, close] : [close, index]; | ||
|
|
||
| const added = new Array(end - start + 1) | ||
| .fill(0) | ||
| .map((_, i) => reverse[i + start]); | ||
|
|
||
| return new Set([...items, ...added]); | ||
| return new Set([...selected, ...added]); |
There was a problem hiding this comment.
Enhanced Logic in addRange Function
The addRange function has been refactored to use the new Records type, which simplifies the handling of record indices. The use of a filtering mechanism to ensure only valid records are processed is a robust approach to error handling. The logic for determining the closest record index and the range of indices to add is clear and well-implemented.
However, the creation of the reverse map (line 26) could potentially be optimized or clarified to ensure its purpose and usage are immediately apparent to future maintainers of this code.
Consider adding a comment explaining the purpose of the reverse map or explore alternative ways to achieve the same functionality with potentially more readable code.
| pageSelector: pageParameters, | ||
| zoomSelector: gridCrop, | ||
| }); | ||
| const records = useRecords(pageReset); |
There was a problem hiding this comment.
Refactor: Introduction of useRecords Hook
The introduction of the useRecords hook to manage the records state is a positive change, enhancing the modularity and reusability of the component. Ensure that the pageReset parameter is correctly handled within the hook to avoid potential issues with state management.
Also applies to: 104-104
| split_info = d[self.split] | ||
| if isinstance(split_info, str): | ||
| split_info = [split_info] | ||
| data_paths = [ | ||
| fos.normpath(os.path.join(dataset_path, si)) for si in split_info | ||
| ] | ||
| classes = _parse_yolo_classes(d.get("names", None)) | ||
|
|
||
| if etau.is_str(data) and data.endswith(".txt"): | ||
| txt_path = _parse_yolo_v5_path(data, self.yaml_path) | ||
| image_paths = [ | ||
| _parse_yolo_v5_path(fos.normpath(p), txt_path) | ||
| for p in _read_file_lines(txt_path) | ||
| ] | ||
| else: | ||
| if etau.is_str(data): | ||
| data_dirs = [data] | ||
| image_paths = [] | ||
| for data_path in data_paths: | ||
| if etau.is_str(data_path) and data_path.endswith(".txt"): | ||
| txt_path = _parse_yolo_v5_path(data_path, self.yaml_path) | ||
| image_paths.extend( | ||
| _parse_yolo_v5_path(fos.normpath(p), txt_path) | ||
| for p in _read_file_lines(txt_path) | ||
| ) |
There was a problem hiding this comment.
Review: Handling of multiple dataset paths in YOLOv5DatasetImporter.setup method.
The modifications in the setup method of the YOLOv5DatasetImporter class enhance its flexibility by allowing it to handle multiple dataset paths. This is achieved by checking if split_info is a string and converting it to a list if necessary, which is then used to construct normalized paths for each dataset split. This approach is beneficial as it supports configurations where the dataset might be split across multiple directories or files, improving the method's usability in diverse scenarios.
However, there are a few considerations and potential improvements:
- Error Handling: The method assumes that the paths provided in
split_infoare valid and that files exist at those paths. It would be prudent to add checks to ensure that the files/directories indeed exist before proceeding with their processing to avoid runtime errors. - Logging: Adding logging at key steps, especially before and after significant operations like file reading, could help in debugging and understanding the flow of data, especially in error scenarios.
- Performance Considerations: If the list of paths is large, the current implementation might become inefficient. Consider parallelizing the reading and processing of paths if performance becomes a bottleneck.
Overall, the changes are a solid improvement, making the method more adaptable to different dataset configurations.
| @drop_datasets | ||
| def test_match_expr(self): | ||
| dataset = _make_group_by_dataset() | ||
|
|
||
| view = dataset.group_by( | ||
| "frame_number", flat=True, match_expr=(F().length() > 1) | ||
| ) | ||
| self.assertDictEqual(view.count_values("frame_number"), {1: 2, 2: 2}) | ||
|
|
There was a problem hiding this comment.
Review: New test method test_match_expr in group_tests.py.
The new test method test_match_expr is designed to validate the functionality of grouping datasets based on a specified expression. The method correctly sets up a dataset, applies a grouping with a match expression that filters elements based on their length, and asserts the expected results.
Key observations:
- Clarity and Scope of Test: The test is clear in its intent and correctly scoped to verify the functionality it targets. It uses the
group_bymethod with amatch_exprthat checks if the length of elements is greater than 1, which is a practical and useful scenario to test. - Correctness of Assertions: The assertion checks the count of values for the
frame_numberkey against the expected results, which effectively validates the behavior of thegroup_bymethod under the conditions set by thematch_expr. - Documentation and Comments: Adding a few comments explaining the choice of
match_exprand why the expected results are{1: 2, 2: 2}would enhance the readability and maintainability of the test, making it easier for new contributors or future maintainers to understand the test's purpose at a glance.
Overall, the addition of this test method is a positive enhancement to the test suite, ensuring that the functionality of grouping by expressions is correctly handled.
Release v0.25.1
Summary by CodeRabbit
New Features
PillButtoncomponent with improved property organization.useRecordshook for better state management of records.SampleWrapperfor improved user interaction.dataCyproperty inPillEntryfor better accessibility and testing.useKeyhook inTabsViewfor improved user interaction tracking.ModalSidebarPomfor enhanced testing capabilities.Bug Fixes
Documentation
Tests
useRecordsanduseSelectSamplefunctionalities.PanelRefclass and new test cases for various functionalities.