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

Merge consensus jobs #8042

Draft
wants to merge 151 commits into
base: gsoc/consensus-feature
Choose a base branch
from

Conversation

Viditagarwal7479
Copy link
Contributor

@Viditagarwal7479 Viditagarwal7479 commented Jun 18, 2024

Fixes Aggregating their annotations and Merging them into the parent job in #7973

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.

Viditagarwal7479 and others added 30 commits May 31, 2024 17:14
…core_threshold which are set through extra configuration when creating task
…can be set through API call also when creating task initially
# Conflicts:
#	cvat-core/src/api.ts
#	cvat-core/src/server-proxy.ts
#	cvat-ui/src/components/actions-menu/actions-menu.tsx
#	cvat-ui/src/components/cvat-app.tsx
#	cvat-ui/src/containers/actions-menu/actions-menu.tsx
#	cvat-ui/src/reducers/index.ts
#	cvat-ui/src/reducers/notifications-reducer.ts
#	cvat/apps/engine/migrations/0079_job_parent_job_id_task_consensus_jobs_per_segment_and_more.py
#	cvat/schema.yml
Copy link
Contributor

@klakhov klakhov left a comment

Choose a reason for hiding this comment

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

Here are my ideas for the consensus settings modal:

  • Notifications. Lets show that the settings are being saved not by additional notification "updating ..." but by adding a loading icon into the save button. Also, I dont think we need Success notification. Lets not show it eighter. Just close the modal when the save is finished.
    image
  • Save/Reset buttons. Need to make 'Save' as 'primary' not 'default. Also, do we really need reset functionality? In quality settings we have lots of settings and dont have reset button. From the other side it can be useful. So, if we want to keep it, I would suggest two things here:
  1. Change the large button for icon
    image from ant icons, and place it in the top of the modal
    image
  2. Instead of resetting right away, we need to confirm this action using simple Modal.confirm
    Also its good to add cancel button just to close the modal. So, in the end we will have 'Save' as primary and 'Cancel' as default buttons in this row.
  • Merge button. Currently it has some problems with styles, text is not white and backrogund becomes white on hover. From my perspective the action button is too large.
    image
    We have a merge icon from the antd. So the bottom menu can look like this:
    image
    We can display merge status above the save/cancel as text. Lock the merge button when the process is active. Also we can show a tooltip on hovering the merge icon. What do you think?

@@ -31,7 +31,7 @@ import Webhook from './webhook';
import { ArgumentError } from './exceptions';
import {
AnalyticsReportFilter, QualityConflictsFilter, QualityReportsFilter,
QualitySettingsFilter, SerializedAsset,
SettingsFilter, SerializedAsset,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we rename this? I think SettingsFilter is too broad term.
For me, it refers to some settings inside cvat annotation view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The QualitySettingsFilter now not only applies to QualitySettings but also ConsensusSettings.

I think SettingsFilter is too broad term

True, I will think about a better name and then discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about AnalyticsSettingsFilter?

cvat-ui/src/components/actions-menu/actions-menu.tsx Outdated Show resolved Hide resolved
cvat-ui/src/actions/consensus-actions.ts Outdated Show resolved Hide resolved
cvat-ui/src/components/consensus/consensus-modal.tsx Outdated Show resolved Hide resolved
@klakhov
Copy link
Contributor

klakhov commented Jul 8, 2024

The consensus tab is not yet ready, am I right?

@Viditagarwal7479
Copy link
Contributor Author

The consensus tab is not yet ready, am I right?

Yes, how it will look is yet to be finalised.

…b` to advanced block in task creation

(cherry picked from commit df460ce44e4039f631953e9ce85ed2531ac760c8)
@Viditagarwal7479
Copy link
Contributor Author

Merge button. Currently it has some problems with styles, text is not white and backrogund becomes white on hover.

This, along with the colour and hover action of the rest of the buttons, has been updated.

In quality settings we have lots of settings and dont have reset button

Should I add a reset button to it, in a separate PR, probably?

If we add the reset button on the top right and the merge button on the bottom left as an icon with a tooltip. In that case, isn't there a possibility that it could be missed by users, and they end up searching for the merge button, or might they never use the reset functionality? Or maybe I am overthinking too much.

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.

None yet

9 participants