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

Display of Annotation Type #1192

Merged
merged 27 commits into from
Jul 24, 2020
Merged

Conversation

Priya4607
Copy link
Contributor

@Priya4607 Priya4607 commented Feb 25, 2020

This PR adds a feature which can help the user distinguish which of the boxes are auto-annotated. When a user annotates an image it is marked as (MANUAL) in each of the boxes created and in case of auto-annotation it is marked as (AUTO). When the user edits a auto-generated box it is changed from (AUTO) to (MANUAL).

  • This currently works in case of auto-annotation using TF Object Detection API
  • In case of undo/redo operations, there is a need to click the button twice to restore the changes.

autro_vs_manual

@Priya4607 Priya4607 changed the title [WIP] Display of Annotation Type Display of Annotation Type Feb 27, 2020
@nmanovic
Copy link
Contributor

@Priya4607 ,

First of all please fix codacy and build (probably you forgot to add migrations). Please check that the patch works in case of undo/redo, propagation, ... When you ready please remove WIP from the patch and we will review and provide feedback.

@nmanovic nmanovic changed the title Display of Annotation Type [WIP] Display of Annotation Type Feb 27, 2020
@Priya4607 Priya4607 changed the title [WIP] Display of Annotation Type Display of Annotation Type Mar 3, 2020
@nmanovic
Copy link
Contributor

nmanovic commented Mar 5, 2020

@azhavoro , could you please review and give your opinion.

Copy link
Contributor

@azhavoro azhavoro left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.
I have question:
What do you think about support of this field for export/import annotations functionality (CVAT format only). For now if I dump and upload anno again I will completely lose auto/manual field for all objects, right?

cvat/apps/engine/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@nmanovic nmanovic left a comment

Choose a reason for hiding this comment

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

@Priya4607 , we have a conflict. Could you please resolve it?

@azhavoro , could you please summarise all your comments into a check list? Thus it will be easy to understand where we are with the PR.

@azhavoro
Copy link
Contributor

azhavoro commented Mar 19, 2020

@Priya4607 thank you for the update
Summarized comments:

  • Add support annotation type for tags

  • Support annotation_type field for CVAT XML dump/upload

  • Fix unit tests & travis build

  • Squash migrations to one file

  • Add info into CHANGELOG.md

Could you please fix the build and squash all migrations into one file?

cvat/apps/annotation/annotation.py Outdated Show resolved Hide resolved
@nmanovic
Copy link
Contributor

nmanovic commented Apr 8, 2020

@Priya4607 , what is your plans regarding the PR?

@Priya4607
Copy link
Contributor Author

@nmanovic
I'll do the changes and update the repo

@Priya4607 Priya4607 requested a review from bsekachev as a code owner April 13, 2020 18:31
@Priya4607 Priya4607 requested a review from zhiltsov-max as a code owner April 17, 2020 17:27
@Priya4607
Copy link
Contributor Author

@azhavoro, @nmanovic With regard to the PR how shall we proceed further?

@azhavoro
Copy link
Contributor

@azhavoro, @nmanovic With regard to the PR how shall we proceed further?

I'll review changes at this week.

cvat/apps/engine/migrations/0026_merge_20200417_1635.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@azhavoro
Copy link
Contributor

I'm ok with the back-end changes (2 minor comments above).
@Priya4607 could you please fix merge conflict?
@bsekachev could you please review the front-end part?

@nmanovic
Copy link
Contributor

@Priya4607 , could you please look at #1767. Do you think you can adapt your changes to the PR? It looks like I will merge it before your PR.

I hope that together we can merge your PR till middle of July. Really appreciate your time and efforts!

@nmanovic
Copy link
Contributor

@azhavoro , please work with @Priya4607 to accept the PR. What should be done for that? Could you please preview it again and provide your comments?

azhavoro
azhavoro previously approved these changes Jul 13, 2020
Copy link
Contributor

@azhavoro azhavoro left a comment

Choose a reason for hiding this comment

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

@Priya4607 thanks for the PR again!
LGTM

Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

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

LGTM, but few recently added tests in cvat/apps/dataset_manager/tests/test_annotation.py should be updated.

migrations.AlterField(
model_name='labeledshape',
name='type',
field=models.CharField(choices=[('rectangle', 'RECTANGLE'), ('polygon', 'POLYGON'), ('polyline', 'POLYLINE'), ('points', 'POINTS'), ('cuboid', 'CUBOID')], max_length=16),
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 have the migration for "type" field here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a migration conflict. So I merged migrations to resolve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Priya4607 , we should have the old migration file and plus new one with only your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I create a new migration, I get the labeledshape model changes added. I'm not sure how to include my changes alone. Could you provide some suggestions on how to proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Priya4607 , it was a problem with develp branch. It missed some migrations. I have fixed that. Could you please update and create a new migration?

@@ -174,6 +174,7 @@ def convert_to_cvat_format(data):
"group": None,
"occluded": False,
"attributes": [],
"annotation_type": "Auto",
Copy link
Contributor

Choose a reason for hiding this comment

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

@Priya4607 , can we rename "annotation_type" into "source". Also I will vote for "auto", "manual" (fist letter should be lower case). In the future we can store in the field even username (or its id).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, should it be applied to both frontend and backend to maintain uniformity?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so.

@nmanovic
Copy link
Contributor

@Priya4607 , we are ready to merge the PR. Let's do the final minor refactoring, update CHANGELOG.md and I will merge. Thanks for the great idea and contribution.

@Priya4607
Copy link
Contributor Author

Priya4607 commented Jul 20, 2020

LGTM, but few recently added tests in cvat/apps/dataset_manager/tests/test_annotation.py should be updated.

@zhiltsov-max Updated test cases

zhiltsov-max
zhiltsov-max previously approved these changes Jul 20, 2020
CHANGELOG.md Outdated
@@ -61,6 +61,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [Datumaro] Added `convert` command to convert datasets directly (<https://github.com/opencv/cvat/pull/1837>)
- [Datumaro] Added an option to specify image extension when exporting datasets (<https://github.com/opencv/cvat/pull/1799>)
- [Datumaro] Added image copying when exporting datasets, if possible (<https://github.com/opencv/cvat/pull/1799>)
- Source type support for tags, shapes and tracks (<https://github.com/opencv/cvat/pull/1192>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong place. Could you please move these lines into the section for next release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@nmanovic
Copy link
Contributor

@Priya4607 , I'm fine with the PR. Need to fix CHANGELOG.md and I will merge. Thanks!

Copy link
Contributor

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- cvat/apps/engine/migrations/0027_auto_20200719_1552.py  1
- cvat-core/src/annotations-objects.js  1
         

Clones removed
==============
+ cvat-core/src/annotations-saver.js  -1
         

See the complete overview on Codacy

CHANGELOG.md Outdated
@@ -467,6 +468,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- Initial version

## [0.6.1] - 2020-04-14
### Added
- Annotation type to determine whether a drawn shape is auto annotated using Tensorflow Object detection API or manually annotated by the user
Copy link
Contributor

Choose a reason for hiding this comment

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

@Priya4607 , some problems with merge. These lines are wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed those lines.

@nmanovic nmanovic merged commit 6a1e7af into cvat-ai:develop Jul 24, 2020
@nmanovic
Copy link
Contributor

@Priya4607 , thanks again for the PR. If you can add the information into statistics about the task (aka info), it will be awesome. Thus we can understand number of manual, interpolated and automatic annotated objects.

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.

6 participants