Initial working version of VOC XML import

[hartmannr76]

Pre-flight checklist

• Unit tests for all non-trivial changes
• Tested locally
• Updated wiki

Confirmed the import works as expected locally. I had a bit of a hard time understanding the expected import format from the onSuccess so I wanted to push an early working version to gather your input to see if this made sense before I started digging into tests.

Please let me know what you think

[SkalskiP]

Hi, @hartmannr76 👋! VOX XML import/export was on my to-do list for a long time. I'm also super excited to see a new contributor! It would be really helpful if you could provide me with a small dataset in VOX XML format so I can test it locally. I imagine that you have something that you use for development.

[hartmannr76]

Hi! Doesn't this project already have VOC export or are you looking to add features to that? Also, I was able to test this with a public dataset. Here is a fairly small one that you're able to export to VOC format with

[codecov-commenter]

Codecov Report

Merging #280 (c99b940) into develop (282ce34) will increase coverage by 0.20%.
The diff coverage is 29.68%.

@@             Coverage Diff             @@
##           develop     #280      +/-   ##
===========================================
+ Coverage    14.51%   14.72%   +0.20%     
===========================================
  Files          153      154       +1     
  Lines         4596     4660      +64     
  Branches       840      845       +5     
===========================================
+ Hits           667      686      +19     
- Misses        3928     3973      +45     
  Partials         1        1              

Impacted Files	Coverage Δ
src/data/ImportFormatData.ts	0.00% <ø> (ø)
src/data/ImporterSpecData.ts	0.00% <ø> (ø)
src/data/enums/AcceptedFileType.ts	100.00% <ø> (ø)
...ws/PopupView/ImportLabelPopup/ImportLabelPopup.tsx	0.00% <ø> (ø)
src/logic/import/voc/VOCImporter.ts	29.68% <29.68%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[SkalskiP]

@hartmannr76 I did the review. Overall code is clean. I did test that and it works. I see that you based the importer on COCOImporter. That is cool, let's just make sure to clean it up from things that don't need to be there. Can't wait for your changes! One more feature to add to make sense in 2022. 🎉

[SkalskiP]

@hartmannr76, let's also update the README.md file in the Import Formats section to reflect that we will now support VOC XML imports. :)

[hartmannr76]

Updated to address comments. I'll add in error handling and tests in the next commit. Few general questions I had for your expectations around some of this -

1. This approach is currently destructive in that the old XML file could have other annotations that wouldn't be included in the export. Is that something you'd like to preserve as an import/export feature for the same file format? Obviously isn't a big deal if a user is exporting to a different format, but may be important for the same (import/export VOC)

2. From what I can tell, none of the importers will re-use existing labels (i.e. each annotation import will delete old labels instead of adding to the existing set). Is this something you'd like to address? I imagine a user should be able to:

• Import image set A
• Import annotations for set A [1, 2, 3]
• Import additional image set B
• Import annotations for set B [2, 4, 5]

With the result being an aggregate annotation set of [1, 2, 3, 4, 5]
This should be easy to add given the existing LabelsSelector.getLabelNames utility but wanted to hear your thoughts

[SkalskiP]

@hartmannr76 I like the changes.

1. I left two more really minor comments about types and names. Nothing really serious.
2. We can definitely test parseAnnotationsFromFileString, method. Anything over that is an extra in my opinion. What do you think?
3. As for error handling. I try to communicate to users the reason for import failure (if that is possible to specify).

• You can use submitNewNotification (src/store/notifications/actionCreators.ts) action to show the new notifications to the user. That action would need to be triggered in onFailure callback that is then passed as an argument to AnnotationImporter.import. Example of that can be found in LoadYOLOv5ModelPopup.txt (src/views/PopupView/LoadYOLOv5ModelPopup/LoadYOLOv5ModelPopup.tsx).

const onFailure = () => {
    setIsLoading(false)
    const notification = modelSource === ModelSource.UPLOAD ?
        Notification.MODEL_LOAD_ERROR : Notification.MODEL_DOWNLOAD_ERROR
    submitNewNotificationAction(NotificationUtil.createErrorNotification(NotificationsDataMap[notification]))
}

I hope it is not too complicated. Let me know if you'd need my help to figure it all out.

[SkalskiP]

@hartmannr76

This approach is currently destructive

Yes, the design is currently not additive. It was done like that to simpliphy the execution. Maybe that is not true for VOC XML, but in principle, for other formats, it would require not only to simply add new names to labelNames but also to validate if labels are consistent across different label file batches, or if different files dont overide current labels. And all of that makes it much more complicated.

All in all, I'm actually thinking about redesigning the import/export system, but let's not focus on that in this PR. However, if you have time to continue makesense.ai contributions then we could meet, discuss and fix all the problems with the import/export mechanism.

[hartmannr76]

@SkalskiP These types of errors are likely similar to existing YOLO and COCO errors and could be genericized but since I'm only using them in this file at the moment just to indicate what imported file had the issue, I was planning to just keep them here for now. Let me know if you'd like me to move it out

[SkalskiP]

Yes. We can keep them here for now. But you are right, long term we could centrally think about the standardization of error types for different label formats, where possible.

[hartmannr76]

We can definitely test parseAnnotationsFromFileString, method. Anything over that is an extra in my opinion. What do you think?

I usually side with tests being exhaustive but I'm also ok with just testing this for now. Added the main expectations for the function

As for error handling. I try to communicate to users the reason for import failure (if that is possible to specify).

Ah, so I meant just to have specific messaging for what went wrong during the import. The notification handling probably should be handled at the ImportLabelPopup file in the onAnnotationsLoadFailure to keep it in one place, right?

However, if you have time to continue makesense.ai contributions then we could meet, discuss and fix all the problems with the import/export mechanism.

I've definitely enjoyed using this so I'd be happy to give input and help where I can. I can't promise too much of a time commitment though as I already have a few side-projects I have on top of my professional work-load 😄

[SkalskiP]

Hi, @hartmannr76! I apologize for being quiet for the last few days. I was not feeling well and needed to rest.

I usually side with tests being exhaustive but I'm also ok with just testing this for now. Added the main expectations for the function

I'm much more relaxed with testing lately. After watching this conversation I tend to not over-focus on testing.

The notification handling probably should be handled at the ImportLabelPopup file in the onAnnotationsLoadFailure to keep it in one place, right?

Yes. You'll get an error as an argument of onAnnotationsLoadFailure, check the type of error and push the appropriate notification.

I can't promise too much of a time commitment though as I already have a few side-projects I have on top of my professional work-load

No worries! I understand. I just love to work with skillful contributors 😅

[SkalskiP]

I guess notifications is the last part that is left :)

[SkalskiP]

Hi, @hartmannr76! The changes look good. Thank you very much for your contribution. I'm merging PR to develop.