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

Added smart attribute keeping when changing label #3309

Merged
merged 2 commits into from
Jul 19, 2021
Merged

Added smart attribute keeping when changing label #3309

merged 2 commits into from
Jul 19, 2021

Conversation

timurlenk07
Copy link
Contributor

Motivation and context

Implements #2755 and #3294. Basically, when changing label class, attributes are retained IF and only there is an attribute with the same name and type, and the old value is still valid in the new configuration.

How has this been tested?

Tested manually on rectangle labels with various attributes.

Checklist

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.
  • I have updated the license header for each file (see an example below)
# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

@timurlenk07 timurlenk07 requested a review from bsekachev as a code owner June 9, 2021 16:50
@timurlenk07
Copy link
Contributor Author

As I work with images, I did not know whether attribute mutability should be considered in the attribute keeping decision. Someone better informed could tell me, what would be the correct way to handle that?

@bsekachev bsekachev self-assigned this Jun 10, 2021
Copy link
Member

@bsekachev bsekachev left a comment

Choose a reason for hiding this comment

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

Hi, sorry for delay in my response.
Generally works fine with shapes and tags, one minor comment about attribute types compatibility. Should we try to keep compatble attributes?

For tracks need to write similar code in Track::_saveLabel at the same file. Do you have an ability to do it? If not, it is okay, we can merge the PR as is.

Also, please add a line to CONTRIBUTING.md and do npm version patch in cvat-core to update its version.

for (const oldAttribute of undoLabel.attributes) {
if (
attribute.name === oldAttribute.name
&& attribute.inputType === oldAttribute.inputType
Copy link
Member

Choose a reason for hiding this comment

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

input types "select" and "radio" are fully compatible
we can also check compatibility additionally between "text" and "radio' or "select"

@timurlenk07
Copy link
Contributor Author

I could make the code to work similarly with tracks, though I won't have the possibility to test it.
Will do the line in CONTRIBUTING.md and the version patch, but I did not understand this question of yours:

Generally works fine with shapes and tags, one minor comment about attribute types compatibility. Should we try to keep compatble attributes?

Could you explain that a bit? Or was it about what you've commented on the code?

@bsekachev
Copy link
Member

bsekachev commented Jun 29, 2021

Generally works fine with shapes and tags, one minor comment about attribute types compatibility. Should we try to keep compatble attributes?

Sure, I will try to rephrase.

As you probably know CVAT supports different types of attributes: CHECKBOX, SELECT, RADIO, TEXT and NUMBER.
Internally SELECT and RADIO are the same meaning (one of a list). So, they are compatible from this point of view and if attributes have the same name (and compatible values), we could try to keep it also regardless of different types.

Speaking about TEXT attributes, we definitely can convert SELECT and RADIO to TEXT with no any issues. And we can do a reverse thing if TEXT attribute value is a possible value for RADIO or SELECT attributes.

@timurlenk07
Copy link
Contributor Author

Now I get it. I will try to do these this week.
Should I rebase onto the latest master commit before doing npm version patch?

@timurlenk07
Copy link
Contributor Author

Now I only check if the attribute names are the same, and if the old value is still valid in the new attribute. This works even for checkboxes, though I am not sure if that is a desired behaviour.

@bsekachev
Copy link
Member

bsekachev commented Jul 19, 2021

@dvkruchinin

Probably we already have a test to change a label, can we update it to cover this case?
Both labels must have an attribute with compatible values.

@bsekachev bsekachev merged commit ba8d064 into cvat-ai:develop Jul 19, 2021
@dvkruchinin
Copy link
Contributor

@dvkruchinin

Probably we already have a test to change a label, can we update it to cover this case?
Both labels must have an attribute with compatible values.

Sure. I'll check and prepare a corresponding task.

@timurlenk07 timurlenk07 deleted the tm/smart-attribute-keeper branch July 26, 2021 15:17
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.

3 participants