[FR] Resolve inconsistencies in label names when importing multiple fields versus a single field

System information

OS Platform and Distribution (e.g., Linux Ubuntu 16.04): Linux Ubuntu 20.04
FiftyOne installed from (pip or source): pip
FiftyOne version (run fiftyone --version): FiftyOne v0.14.2, Voxel51, Inc.
Python version: Python 3.9.7

Describe the problem

There's a inconsistency for loading COCO dataset when using include_id=True. By default include_id is False, and loading a COCO dataset inherently loads the dataset by creating "ground_truth" field for samples and "detections" as a sub-field of "ground_truth" which is perfectly fine. However, when we try to preserve COCO ids with include_id=True, the name for ground_truth detections are now created as "detections" field and "detections" as a subfield, and it saves the COCO ids in "coco_id" field. To better illustrate the nested form, it can be shown as follows:

{
    ...,
    "detections": { "detections": [...]},
    "coco_id": 174,
}

The first problem is the naming inconsistency between "ground_truth" and "detections" when you switch from include_id=False to include_id=True (subfields' name remain as "detections" for both). The expected behaviour is to get the same field name whether or not using include_id as False/True. Imho, "ground_truth" is better naming convention here.

The second problem is that when we try to fix this problem with label_field parameter, e.g label_field="ground_truth", it makes things worse as the new field name is concatenated string "<label_field> + detections", so if our label_field is "ground_truth" the detections field becomes "ground_truth_detections". Moreover, it also does the same to the field "coco_id" as it becomes "ground_truth_coco_id".

Code to reproduce issue

The behaviour described above can be observed with the following code.

coco_dataset = fo.Dataset.from_dir(
    name="coco-dataset",
    dataset_type=fo.types.COCODetectionDataset,
    dataset_dir=IMAGES_DIR,
    labels_path="/path/to/coco.json",
    label_field="ground_truth",
    include_id=True
)

>>> print(sample)
<...
"ground_truth_detections": "detections": <[...]>,
"ground_truth_coco_id": 174
...>

What areas of FiftyOne does this bug affect?

[ ] App: FiftyOne application issue
[x] Core: Core fiftyone Python library issue
[ ] Server: Fiftyone server issue

Willingness to contribute

The FiftyOne Community encourages bug fix contributions. Would you or another member of your organization be willing to contribute a fix for this bug to the FiftyOne codebase?

[ ] Yes. I can contribute a fix for this bug independently.
[ ] Yes. I would be willing to contribute a fix for this bug with guidance from the FiftyOne community.
[x] No. I cannot contribute a bug fix at this time.

What you're seeing is standard practice throughout the codebase. Whenever a single label field is being loaded through an importer, the default field name is ground_truth, which we agree is the better naming convention. However, when an importer loads multiple label fields at the same time, then we need to distinguish the field names in some way.

For example, when loading both detections and segmentations label types for a COCO dataset, it is not clear which should be ground_truth, so they are loaded as "detections" and "segmentations". Even if you specify the label_field parameter, it is still unclear which should be renamed to ground_truth, so it is simply prepended to the field names.

coco_dataset = fo.Dataset.from_dir(
    name="coco-dataset",
    dataset_type=fo.types.COCODetectionDataset,
    dataset_dir=IMAGES_DIR,
    labels_path="/path/to/coco.json",
    label_types=["detections", "segmentations"],
)

To work around this currently, you can easily rename the detections field to ground_truth using rename_sample_field():

dataset.rename_sample_field("detections", "ground_truth")

A feature that we could consider is to allow the label_field parameter to accept a dictionary mapping the specific default field names to field names you provide. This would disambiguate which field should be given the provided label_field name. For example:

coco_dataset = fo.Dataset.from_dir(
    name="coco-dataset",
    dataset_type=fo.types.COCODetectionDataset,
    dataset_dir=IMAGES_DIR,
    labels_path="/path/to/coco.json",
    label_field={"detections": "ground_truth"},
    include_id=True
)

>>> print(sample)
<...
"ground_truth": "detections": <[...]>,
"coco_id": 174
...>

Thanks for the response @ehofesmann. Yet, I still don't understand this statement,

For example, when loading both detections and segmentations label types for a COCO dataset, it is not clear which should be ground_truth, so they are loaded as "detections" and "segmentations".

Why so? I mean the default behavior is to apply field "ground_truth" and put "detections" under this field, so it becomes nested like sample["ground_truth"]["detections"], which is great actually, so since detections field is put under the field "ground_truth", we can also put "segmentations" under this "ground_truth" field, am I right?

Thus, I believe this shouldn't be causing problems between detections/segmantation because both can put under a single label_field. Isn't this the correct behavior if I do not miss something; for example, if in fields there cannot be more than one subfield, then your statement is understandable although I'm not sure it is reasonable.

So at this stage, can you confirm that there could only be 1 subfield for each field ? @ehofesmann

So at this stage, can you confirm that there could only be 1 subfield for each field ? @ehofesmann

Yes, each field can only have one "subfield". Though to hopefully clarify some things, "ground_truth" is not an arbitrary dictionary field that we create and decided to put a list of "detections" under. When loading COCO object detections, "ground_truth" is created as an fo.Detections type field which happens to have a detections property that stores a list of fo.Detection objects.

The FiftyOne API is flexible enough to let you store arbitrary Python lists and dictionaries in fields if you want. However, in order for the App to be able to visualize your data, it needs to be more structured. The App can visualize your data if it is stored in a field is of a primitive data type like IntField or StringField or a more complex Label type.

For example, "detections" may be a field with labels of type fo.Detections which is defined to have a detections property that stores a list of individual fo.Detection objects. On the other hand, "segmentations" may be a field of type fo.Polylines which has a polylines property that stores a list of individual fo.Polyline segmentations. These are distinct label types which is why a list of polylines cannot be added to a fo.Detections type field.

@ehofesmann Oh okay, I initially assumed that fields can have multiple subfields. Thanks for the clarification though I still have one more question regarding this problem.

The default behavior of importing a COCO dataset is to load only detections and not segmentations, and thus it can import the dataset with a field "ground_truth" and detections under it. Now, I do nothing else but only add include_id=True, now I expect the same behavior (loading only detections) to be in effect, indeed it only load detections and not segmentations (actually I did not try with a file containing segmentations though, so not sure about this), however it changes the "ground_truth" name as "detections". Here, I'd expect the dataset to be imported with only detections (default behavior) where the detections lie under "ground_truth" field, but this is not the case. In short, switching from include_id=False to include_id=True (while other params held fixed) changes the behavior of the import. In other words, it's like with include_id=True it assumes that it's importing segmentations/detections, but with include_id=False by default it only assumes to load detections.

This behavior might also be caused by addition of another field "coco_id". I mean it may still assume to load detections only; however, since there are more than one fields changed ("detections" and "coco_id"), it probably alters the field names.

All in all, I was trying to understand this behavior throughout, so I think it's more of a bug, with a follow up FR, as it causes poor design in code base.

This behavior might also be caused by addition of another field "coco_id". I mean it may still assume to load detections only; however, since there are more than one fields changed ("detections" and "coco_id"), it probably alters the field names.

You are correct here. Every LabeledDatasetImporter needs to define a label_cls property which can return a dictionary of all the field names and field types that are returned by the importer. https://github.com/voxel51/fiftyone/blob/7d1fa672a81547f2284704d24417e246bb071c0b/fiftyone/utils/coco.py#L507-L513

Whenever two or more of these fields are returned, it uses the field names instead of "ground_truth" due to ambiguity in what should be the "ground_truth". You can see that "coco_id" and "detections" are both equally valid return fields specified in the label_cls for the COCODetectionDatasetImporter meaning we can't assume that either should be "ground_truth" meaning we need to use the field names instead.

It seems that the primary point of confusion is that the "detections", "segmentations", and "keypoints" fields are specified through the label_types parameter while "coco_id" and "license" fields are specified through individual keyword arguments include_id and include_license since they are not technically a type of fo.Label.

The default behavior of importing a COCO dataset is to load only detections and not segmentations

In hindsight, it would likely have been better to automatically load all label types (detections, segmentations, and keypoints) if they are found rather than only loading detections.

It seems that the primary point of confusion is that the "detections", "segmentations", and "keypoints" fields are specified through the label_types parameter while "coco_id" and "license" fields are specified through individual keyword arguments include_id and include_license since they are not technically a type of fo.Label.

Yes, exactly the confusion arises due to this. Since "coco_id" and "license" fields are sample related fields, they shouldn't cause any contradiction for labels. I'd expect import behavior changes only if multiple of {"detections", "segmentations", "keypoints"} fields (label fields) are imported, but addition of "coco_id" or "license" should not affect this behavior.

So, at this point we've reached to the final discussion I believe, whether or not this should be handled inherently or not (bug/feature) ?

Yes, exactly the confusion arises due to this. Since "coco_id" and "license" fields are sample related fields, they shouldn't cause any contradiction for labels. I'd expect import behavior changes only if multiple of {"detections", "segmentations", "keypoints"} fields (label fields) are imported, but addition of "coco_id" or "license" should not affect this behavior.

So, at this point we've reached to the final discussion I believe, whether or not this should be handled inherently or not (bug/feature) ?

First off, thanks for the discussion, it has been very helpful to think through what should be the default behavior. In my opinion, this is more of a code enhancement than a feature or a bug. However, since this is the standard practice in much of the code base, we need to think a bit more about the best way to approach this. It may be best to just avoid loading "ground_truth" altogether and always load the specific label name.

Hello , if i want to get the segmentation and the detection labels , what changes do i need to do to this function ? ( it's only exporting detection labels

def export_yolo_data( samples, export_dir, classes, label_field = "ground_truth", split = None ): if type(split) == list: splits = split for split in splits: export_yolo_data( samples, export_dir, classes, label_field, split ) else: if split is None: split_view = samples split = "val" else: split_view = samples.match_tags(split)

    split_view.export(
        export_dir=export_dir,
        dataset_type=fo.types.YOLOv5Dataset,
        label_field=label_field,
        classes=classes,
        split=split
    )

voxel51 / fiftyone