Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GSoC'24] Consensus annotation #8434

Open
wants to merge 350 commits into
base: develop
Choose a base branch
from
Open

Conversation

zhiltsov-max
Copy link
Contributor

@zhiltsov-max zhiltsov-max commented Sep 11, 2024

Motivation and context

Depends on #8283, #8401

Closes #7973

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

Summary by CodeRabbit

  • New Features

    • Introduced a new analytics page for consensus reporting.
    • Added support for merging consensus jobs directly from the job actions menu.
    • Implemented a dedicated component for displaying assignee reports and consensus conflicts.
    • Enhanced task management with new consensus-related settings and properties.
  • Bug Fixes

    • Improved error handling and state management for consensus job operations.
  • Documentation

    • Updated documentation to reflect changes in consensus job handling and analytics features.
  • Style

    • Added new styles for improved layout and presentation of consensus-related components.
  • Tests

    • Expanded test coverage for new consensus functionalities and components.

…b` to advanced block in task creation

(cherry picked from commit df460ce)

- Quality analytics page will now report job assignees from quality reports
instead of current job assignees
(<https://github.com/cvat-ai/cvat/pull/8123>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect merge

@@ -0,0 +1,4 @@
### Added

- Datumaro format now supports skeletons
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect merge

Please, add correct changelog entry, describing what was implemented in this patch

Comment on lines +71 to +72
frameConflicts.forEach((qualityConflict: T) => {
const { type, serverID } = qualityConflict.annotationConflicts[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to remove comments that was left in original function?

If not, please return them


const params = fieldsToSnakeCase({ ...filter, sort: '-id' });

const reportsData = await serverProxy.consensus.assignee_reports(params);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use camelCase naming style on client. assignee_reports -> assigneeReports

},
});

return response.data.results[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we always take the first item from the list?
Why not the last one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being used for consensus settings and quality settings for a given filter. We would get a list containing a single element.

Comment on lines 13 to 14
import { useSelector } from 'react-redux';
import { LoadingOutlined } from '@ant-design/icons';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3rdparty imports should come before internal imports

Comment on lines +112 to +115
window.addEventListener('hashchange', () => {
const hash = getTabFromHash();
setTab(hash);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any event listener should be destroyed in useEffect's return. Otherwise this is memory leak

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this. Correct?

return () => {
    window.removeEventListener('hashchange', () => {
        setTab(getTabFromHash());
    });
}

I guess I also make this change in cvat-ui/src/components/analytics-page/analytics-page.tsx

@Viditagarwal7479
Copy link
Contributor

If this patch also uses them, need to re-store and update our internal code to reduce code duplication

Hello @bsekachev, how would you suggest to proceed further on this?

@Viditagarwal7479
Copy link
Contributor

I have made changes to fix the rest of the reviews on which I haven't commented.
Once we figure out how to fix the breaks due to sharing components from quality analytics with consensus analytics, I shall merge them!

Copy link

sonarcloud bot commented Oct 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5.7% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GSoC'24] Quality Control: Consensus
3 participants