Fixed propagate in ground truth tasks with sparsed frames #8550
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduced in this pull request focus on enhancing the propagation mechanism and frame handling within the annotation framework. Key updates include the introduction of new methods for frame number retrieval, modifications to existing methods for better type safety and flexibility, and improvements in state management within the UI components and reducers. The changes aim to ensure that frame-related operations are performed accurately and efficiently, while also enhancing the overall clarity and robustness of the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant AnnotationService
participant Job
participant Task
User->>UI: Initiate shape propagation
UI->>AnnotationService: propagateObjectAsync(from, to)
AnnotationService->>Job: Validate frame indices
Job->>AnnotationService: Return valid frame numbers
AnnotationService->>UI: Dispatch success action with frame numbers
UI->>User: Display propagation result
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
CodeRabbit Configuration 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: 11
🧹 Outside diff range and nitpick comments (9)
cvat-ui/src/components/annotation-page/standard-workspace/propagate-confirm.tsx (3)
103-106: Enhanced type safety in InputNumber onChange handlerThe change to accept
number | nulland check fornumbertype improves type safety. This prevents potential runtime errors from null inputs.Consider using a default value or early return for null inputs to make the logic more explicit:
onChange={(value: number | null) => { if (value === null) return; // or use a default value updateTargetFrame(propagateDirection, value); }}
Line range hint
143-153: Improved type safety and logic in second InputNumber onChange handlerThe changes to accept
number | nulland the more detailed logic for setting the target frame are good improvements. They enhance type safety and handle different input scenarios more comprehensively.For consistency with the earlier InputNumber, consider using an early return for null inputs:
onChange={(value: number | null) => { if (value === null) return; if (value > frameNumber) { setTargetFrame(clamp(value, frameNumber, stopFrame)); } else if (value < frameNumber) { setTargetFrame(clamp(value, startFrame, frameNumber)); } else { setTargetFrame(frameNumber); } }}
Line range hint
1-159: Consider refactoring repeated clamp logicThe changes made improve type safety and state management consistently throughout the component. As a further improvement, consider refactoring the repeated clamp logic into a utility function to reduce code duplication and improve maintainability.
You could create a utility function like this:
const clampTargetFrame = (value: number, reference: number): number => { if (value > reference) { return clamp(value, reference, stopFrame); } if (value < reference) { return clamp(value, startFrame, reference); } return reference; };Then use it in both InputNumber onChange handlers:
onChange={(value: number | null) => { if (value === null) return; setTargetFrame(clampTargetFrame(value, frameNumber)); }}This would make the code more DRY and easier to maintain.
cvat-core/src/session.ts (4)
375-375: LGTM! Consider documenting the default behavior.The change to make the
countparameter optional in theundomethod is a good improvement, providing more flexibility in usage. Users can now call the method without explicitly specifying how many actions to undo.Consider adding a comment or updating the documentation to clarify the default behavior when
countis not provided. For example, does it undo one action by default?
376-376: LGTM! Consider documenting the default behavior.The change to make the
countparameter optional in theredomethod is a good improvement, providing more flexibility in usage and consistency with theundomethod.Similar to the
undomethod, consider adding a comment or updating the documentation to clarify the default behavior whencountis not provided. For example, does it redo one action by default?
405-406: LGTM! Consider documenting default values and behavior.The changes to make the
payloadandwaitparameters optional in thelogmethod are good improvements, providing more flexibility in logging. Users can now call the method with only the requiredscopeparameter if payload and wait are not needed.Consider the following suggestions:
- Add comments or update documentation to clarify the default values or behavior when
payloadandwaitare not provided.- If applicable, consider using default parameter values in the method signature for clarity, e.g.,
payload: Parameters<typeof logger.log>[1] = {}.- Ensure that the implementation of the
logmethod handles these optional parameters correctly.
Line range hint
375-406: Overall improvements in API flexibilityThe changes in this file enhance the flexibility of the Session class (and by extension, the Job and Task classes) by making certain method parameters optional. This includes:
- Making the
countparameter optional for bothundoandredomethods in theactionsproperty.- Making the
payloadandwaitparameters optional for thelogmethod in theloggerproperty.These modifications allow for more concise method calls when default behavior is desired, while still maintaining the option to specify these parameters when needed. This should improve the developer experience when working with these classes.
To further improve the codebase:
- Ensure that these changes are reflected in the documentation, including any default behaviors.
- Consider applying similar flexibility improvements to other parts of the API where it makes sense.
- If not already done, update any unit tests to cover both the cases where these parameters are provided and when they are omitted.
cvat-ui/src/actions/annotation-actions.ts (1)
467-469: Consider logging or handling whensessionInstanceis undefined.While throwing an error when
sessionInstanceis undefined prevents further execution, it might be helpful to log this event or provide additional context to aid in debugging.Apply this diff to enhance error handling:
if (!sessionInstance) { + console.error('SessionInstance is undefined in propagateObjectAsync.'); throw new Error('SessionInstance is not defined; propagation is not possible.'); }cvat-core/src/session-implementation.ts (1)
883-883: Throw a more specific errorInstead of throwing a generic
Error, consider creating and throwing a custom error or a more specific built-in error (e.g.,NotImplementedError) to provide more context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- changelog.d/20241016_133802_sekachev.bs_fixed_propagation.md (1 hunks)
- cvat-core/src/annotations-actions.ts (2 hunks)
- cvat-core/src/frames.ts (3 hunks)
- cvat-core/src/object-utils.ts (2 hunks)
- cvat-core/src/session-implementation.ts (1 hunks)
- cvat-core/src/session.ts (2 hunks)
- cvat-ui/src/actions/annotation-actions.ts (5 hunks)
- cvat-ui/src/components/annotation-page/standard-workspace/propagate-confirm.tsx (4 hunks)
- cvat-ui/src/reducers/annotation-reducer.ts (3 hunks)
- cvat-ui/src/reducers/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- changelog.d/20241016_133802_sekachev.bs_fixed_propagation.md
🧰 Additional context used
🔇 Additional comments (12)
cvat-ui/src/components/annotation-page/standard-workspace/propagate-confirm.tsx (2)
26-34: Improved state management and frame handlingThe refactoring of the
useSelectorhook and the derivation ofstartFrameandstopFramefrom theframeNumbersarray are good improvements. This approach centralizes state selection and makes the component more flexible to changes in frame numbering.Also applies to: 36-38
Line range hint
125-132: Simplified Slider onChange handlerThe change to accept an array of numbers instead of a tuple simplifies the code and makes it more flexible. The logic for determining which value to use is clear and correct.
cvat-ui/src/reducers/index.ts (2)
Line range hint
1-1186: Overall assessment of changes to AnnotationStateThe modifications to the
AnnotationStateinterface, specifically thejobobject, appear to be part of a larger feature implementation or enhancement related to frame handling and job initialization. The changes are type-safe and consistent with the existing code style.To ensure a smooth integration of these changes:
- Review and update any components or reducers that interact with the
jobobject in theAnnotationState.- Update relevant documentation to reflect these new properties.
- If there are corresponding backend changes, ensure they align with these new frontend properties.
- Consider adding unit tests to cover scenarios involving these new properties.
Line range hint
727-733: New properties added to the job object in AnnotationStateThe following changes have been made to the
jobobject within theAnnotationStateinterface:
- A new
frameNumbersproperty of typenumber[]has been added.- New query parameters have been added to the
queryParametersobject:
defaultLabelof typestring | nulldefaultPointsCountof typenumber | nullThese additions suggest enhancements in how frame data and job initialization are handled. Consider the following:
- Ensure that any components or functions that interact with the
jobobject are updated to handle these new properties.- Update any relevant documentation to reflect these changes.
- If there are corresponding changes in the backend or API, make sure they align with these new properties.
To verify the usage and impact of these new properties, please run the following script:
This script will help identify any places where these new properties are being used or where they might need to be implemented.
✅ Verification successful
Verification Successful: No Code Removals or Replacements Detected
The added properties
frameNumbers,defaultLabel, anddefaultPointsCountincvat-ui/src/reducers/index.tshave been correctly integrated and are actively used throughout the codebase. No removed or replaced code was found in the specified lines.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new job properties across the codebase echo "Checking for frameNumbers usage:" rg --type typescript "frameNumbers" ./cvat-ui/src echo "\nChecking for defaultLabel usage:" rg --type typescript "defaultLabel" ./cvat-ui/src echo "\nChecking for defaultPointsCount usage:" rg --type typescript "defaultPointsCount" ./cvat-ui/srcLength of output: 505
Script:
#!/bin/bash # Description: Check for usage of new job properties across the codebase echo "Checking for frameNumbers usage:" rg -g "*.ts" -g "*.tsx" "frameNumbers" ./cvat-ui/src echo "" echo "Checking for defaultLabel usage:" rg -g "*.ts" -g "*.tsx" "defaultLabel" ./cvat-ui/src echo "" echo "Checking for defaultPointsCount usage:" rg -g "*.ts" -g "*.tsx" "defaultPointsCount" ./cvat-ui/srcLength of output: 7273
cvat-core/src/frames.ts (4)
214-217: LGTM: New methodgetSegmentFrameNumberslooks good.The new
getSegmentFrameNumbersmethod in theFramesMetaDataclass is a well-implemented addition. It correctly utilizes existing methods to convert data frame numbers to job-relative frame numbers, maintaining consistency with the current codebase structure.
356-356: Improvement: Simplified frame number calculation.The use of the new
getSegmentFrameNumbersmethod here improves code readability and maintainability. It encapsulates the frame number calculation logic, making the code more modular and easier to understand.
895-895: Improvement: SimplifiedgetJobFrameNumbersfunction.The
getJobFrameNumbersfunction has been refactored to use the newgetSegmentFrameNumbersmethod. This change reduces code duplication and improves consistency across the codebase. The functionality remains the same while the code becomes more maintainable.
214-217: Summary: Excellent refactoring for frame number handling.The changes introduced in this file represent a well-executed refactoring of frame number handling. The new
getSegmentFrameNumbersmethod in theFramesMetaDataclass centralizes the logic for converting between data frame numbers and job-relative frame numbers. This method is then consistently used throughout the file, including in theFrameData.prototype.dataimplementation and thegetJobFrameNumbersfunction.These changes result in:
- Improved code readability
- Reduced duplication of logic
- Better maintainability
- Consistent handling of frame numbers across different parts of the code
Overall, this refactoring enhances the quality of the codebase without introducing breaking changes or potential issues.
Also applies to: 356-356, 895-895
cvat-core/src/annotations-actions.ts (2)
110-110:⚠️ Potential issueEnsure method signature matches the base class for type safety
The method signature of
runinPropagateShapesshould match the base classBaseSingleFrameActionto ensure type safety and proper method overriding.Consider updating the method signature to:
-public async run( +public async run( instance: Job | Task, { collection: { shapes }, frameData: { number } }, ): Promise<SingleFrameActionOutput> {Alternatively, ensure that the base class method is correctly overridden.
Likely invalid or redundant comment.
117-117:⚠️ Potential issueVerify frame number range for
Taskinstances to prevent off-by-one errorsWhen generating frame numbers for
Taskinstances usingrange(0, instance.size), ensure that the frame numbering starts at0and ends atinstance.size - 1. Sincelodash'srangefunction generates numbers up to, but not including, the end value, there is a potential for off-by-one errors ifinstance.sizedoes not accurately represent the total number of frames.Consider running the following script to verify that the frame numbers are correctly generated for
Taskinstances:✅ Verification successful
Frame Number Range Verified
The frame numbering for
Taskinstances correctly starts at0and ends atinstance.size - 1, preventing any off-by-one errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that frame numbers in Task instances start at 0 and end at (instance.size - 1) # Test: Check the frame numbers for a Task instance # Expected Result: Frame numbers should range from 0 to (instance.size - 1) # Fetch all Task instances and display their sizes rg --type typescript 'class Task' -A 20 | rg 'size' # Verify the implementation of frame numbering in Task instances rg --type typescript 'Task.prototype.frames' -A 20Length of output: 172
Script:
#!/bin/bash # Description: Verify that frame numbers in Task instances start at 0 and end at (instance.size - 1) # Test: Check the frame numbers for a Task instance # Expected Result: Frame numbers should range from 0 to (instance.size - 1) # Fetch all Task instances and display their sizes rg --glob '*.ts' 'class Task' -A 20 | rg 'size' # Verify the implementation of frame numbering in Task instances rg --glob '*.ts' 'Task.prototype.frames' -A 20Length of output: 12909
cvat-core/src/object-utils.ts (1)
363-363: Ensure all usages ofpropagateShapesare updated to the new signatureThe function
propagateShapesnow acceptsframeNumbers: number[]instead ofto: number. Please verify that all calls to this function throughout the codebase are updated accordingly to prevent any runtime errors.cvat-ui/src/actions/annotation-actions.ts (1)
453-453: EnsureframeNumbersis available in the state before destructuring.When destructuring
frameNumbersfromstate.annotation.job, confirm thatframeNumbersis initialized and populated to avoid potentialundefinedvalues.
| @@ -107,13 +107,15 @@ class PropagateShapes extends BaseSingleFrameAction { | |||
| } | |||
|
|
|||
| public async run( | |||
| instance, | |||
| instance: Job | Task, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent parameter naming with the base class
In the run method of PropagateShapes, consider renaming the parameter instance to sessionInstance to maintain consistency with the base class BaseSingleFrameAction. This enhances readability and makes it easier for developers to follow the codebase.
| @@ -2,7 +2,7 @@ | |||
| // | |||
| // SPDX-License-Identifier: MIT | |||
|
|
|||
| import { omit, throttle } from 'lodash'; | |||
| import { omit, range, throttle } from 'lodash'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import only necessary functions to optimize bundle size
While importing range from lodash, consider importing only the specific functions you need to reduce the bundle size. This can be done by importing from lodash/range instead:
-import { omit, range, throttle } from 'lodash';
+import omit from 'lodash/omit';
+import range from 'lodash/range';
+import throttle from 'lodash/throttle';This ensures that only the required modules are included, improving the performance of the application.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { omit, range, throttle } from 'lodash'; | |
| import omit from 'lodash/omit'; | |
| import range from 'lodash/range'; | |
| import throttle from 'lodash/throttle'; |
| { collection: { shapes }, frameData: { number } }, | ||
| ): Promise<SingleFrameActionOutput> { | ||
| if (number === this.#targetFrame) { | ||
| return { collection: { shapes } }; | ||
| } | ||
| const propagatedShapes = propagateShapes<SerializedShape>(shapes, number, this.#targetFrame); | ||
|
|
||
| const frameNumbers = instance instanceof Job ? await instance.frames.frameNumbers() : range(0, instance.size); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using native JavaScript methods for generating ranges
Instead of using lodash's range function, you might use native JavaScript array methods to generate the range of frame numbers. This can reduce external dependencies and improve performance.
Example:
-const frameNumbers = instance instanceof Job ? await instance.frames.frameNumbers() : range(0, instance.size);
+const frameNumbers = instance instanceof Job
+ ? await instance.frames.frameNumbers()
+ : Array.from({ length: instance.size }, (_, i) => i);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const frameNumbers = instance instanceof Job ? await instance.frames.frameNumbers() : range(0, instance.size); | |
| const frameNumbers = instance instanceof Job | |
| ? await instance.frames.frameNumbers() | |
| : Array.from({ length: instance.size }, (_, i) => i); |
cvat-core/src/object-utils.ts
Outdated
| for (const frame of frameNumbers) { | ||
| if (frame === from) { | ||
| continue; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider filtering frameNumbers before the loop to exclude from
Currently, the loop checks if frame === from and uses continue to skip the iteration. For better readability and efficiency, consider filtering out the from frame from frameNumbers before entering the loop.
Apply this change:
const states: T[] = [];
+ const targetFrames = frameNumbers.filter((frame) => frame !== from);
- for (const frame of frameNumbers) {
+ for (const frame of targetFrames) {
// existing loop code
}Committable suggestion was skipped due to low confidence.
| @@ -66,6 +66,7 @@ const defaultState: AnnotationState = { | |||
| groundTruthJobFramesMeta: null, | |||
| groundTruthInstance: null, | |||
| }, | |||
| frameNumbers: [], | |||
There was a problem hiding this comment.
Ensure TypeScript interfaces include frameNumbers in job state
The addition of frameNumbers to the job state should be reflected in the TypeScript interfaces (AnnotationState and any related interfaces) to maintain type safety and prevent potential type errors.
| @@ -205,6 +207,7 @@ | |||
| job: { | |||
| ...state.job, | |||
| openTime, | |||
| frameNumbers, | |||
There was a problem hiding this comment.
Validate frameNumbers before updating the state
Before assigning frameNumbers to the job state, consider validating that it is properly defined and contains the expected array of frame numbers to avoid any unexpected behavior.
| const jobMeta = await cvat.frames.getMeta('job', job.id); | ||
| const frameNumbers = await job.frames.frameNumbers(); |
There was a problem hiding this comment.
Add error handling for external calls to getMeta and frameNumbers.
The calls to cvat.frames.getMeta('job', job.id) and job.frames.frameNumbers() may fail due to network issues or server errors, potentially causing unhandled exceptions. Consider wrapping these calls in try-catch blocks to handle possible errors gracefully and provide informative error messages.
Apply this diff to add error handling:
const [job] = await cvat.jobs.get({ jobID });
+ let jobMeta;
+ let frameNumbers;
+ try {
+ jobMeta = await cvat.frames.getMeta('job', job.id);
+ frameNumbers = await job.frames.frameNumbers();
+ } catch (error) {
+ throw new Error(`Failed to retrieve job metadata or frame numbers: ${error.message}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const jobMeta = await cvat.frames.getMeta('job', job.id); | |
| const frameNumbers = await job.frames.frameNumbers(); | |
| let jobMeta; | |
| let frameNumbers; | |
| try { | |
| jobMeta = await cvat.frames.getMeta('job', job.id); | |
| frameNumbers = await job.frames.frameNumbers(); | |
| } catch (error) { | |
| throw new Error(`Failed to retrieve job metadata or frame numbers: ${error.message}`); | |
| } |
| const framesToPropagate = frameNumbers.filter( | ||
| (frameNumber: number) => frameNumber >= Math.min(from, to) && frameNumber <= Math.max(from, to), | ||
| ); |
There was a problem hiding this comment.
Verify that frameNumbers is defined and not empty before filtering.
When filtering frameNumbers to obtain framesToPropagate, ensure that frameNumbers is defined and contains frame numbers. If frameNumbers is undefined or empty, framesToPropagate will also be undefined or empty, which may lead to unexpected behavior.
Apply this diff to add a check:
const framesToPropagate = frameNumbers.filter(
(frameNumber: number) => frameNumber >= Math.min(from, to) && frameNumber <= Math.max(from, to),
);
+ if (!framesToPropagate.length) {
+ throw new Error('No frames available for propagation.');
+ }Committable suggestion was skipped due to low confidence.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8550 +/- ##
===========================================
+ Coverage 74.28% 74.30% +0.02%
===========================================
Files 401 401
Lines 43405 43419 +14
Branches 3945 3951 +6
===========================================
+ Hits 32242 32262 +20
+ Misses 11163 11157 -6
|
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
These updates improve the overall robustness and user experience within the annotation framework.