Skip to content

Added mmdet smqtk plugin implementation, tests, and config files#39

Open
alexander-lynch wants to merge 2 commits intoKitware:masterfrom
alexander-lynch:mmdetection_plugin
Open

Added mmdet smqtk plugin implementation, tests, and config files#39
alexander-lynch wants to merge 2 commits intoKitware:masterfrom
alexander-lynch:mmdetection_plugin

Conversation

@alexander-lynch
Copy link

Moved this work to smqtk instead of learn https://gitlab.kitware.com/darpa_learn/learn/-/merge_requests/179

@Purg
Copy link
Member

Purg commented Sep 12, 2022

Going to give this a rebase poke since I needed to reactivate the actions workflows.

@Purg Purg force-pushed the mmdetection_plugin branch from e8b4737 to 30cb555 Compare September 12, 2022 17:52
Comment on lines +11 to +14
import mmcv

import torch.nn
from torch.utils.data import DataLoader, Dataset, IterableDataset
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is "optional" in this package as these dependencies are not requirements on the base package (they should continue to not be base package requirements). We will need to add protection to imports of these implementation-specific requirements. See other impls here like smqtk_detection/impls/detect_image_objects/resnet_frcnn.py.

We should also update the pyproject.yaml to include optional dependencies and an extra specification for this mmdet plugin.

Comment on lines +29 to +31
It is expected that classes will be derived from this base that concretely
defines the data augmentation to appropriate transform input imagery for
the configured network.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this applies anymore for this implementation (copy/paste from detectron2 plugin).

Comment on lines +133 to +134
if len(batch) < self._batch_size:
continue
Copy link
Member

Choose a reason for hiding this comment

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

This will only handle when the input and batch size are evenly divisible. If there is a partial batch left over it will not currently be processed. A post-loop partial-batch condition will need to be added after this top-level for-loop.

Comment on lines +37 to +60
This plugin attempts to be intelligent in how it handles different kinds of
iterable inputs. When given a ``Dataset`` or countable sequence (has
``__len__`` and ``__getitem__``), any valid value may be provided to
``num_workers`` as ``DataLoader`` might accept.
However, when the input to ``detect_objects`` is an uncountable iterable,
like a generic generator or stream source, the ``num_workers`` value should
usually be either 0 or 1.
This is due to the input iterable being copied for each worker, which may
not result in desired behavior.
For example:
* when the input iterable involves non-trivial operations per yield,
these operations are duplicated for each copy of the iterable as
traversed on each worker, probably resulting in excessive use of
resources. E.g. if the iterable is loading images from disk, each
worker is loading every image as it traverses their copy of the
iterable, even though each worker may only operate on a minority of
elements traversed.
* when the input iterable yields real-time data or is otherwise **not**
idempotent, like an iterable that yields images from a webcam stream,
each traversal of a copy of that iterable will produce different values
for equivalent "indices" since what is returned is conditional on when
``next()`` is requested. Since iterators are copied to N separate
workers, each making independent next requests, the e.g. 64th
``next()`` for each worker might yield a different image matrix.
Copy link
Member

Choose a reason for hiding this comment

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

This whole section of the doc-string doesn't seem to apply anymore.

self._batch_size = batch_size
self._weights_uri = weights_uri
self._model_lazy_load = model_lazy_load
self._num_workers = num_workers
Copy link
Member

Choose a reason for hiding this comment

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

Num workers does not need to apply any more. We should remove this constructor variable/doc and the use in config return.

return torch.as_tensor(aug_image)


def _aug_one_image(image: np.ndarray, aug: AutoAugment, gt_bboxes=[]) -> Dict[str, Union[torch.Tensor, int]]:
Copy link
Member

Choose a reason for hiding this comment

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

This function does not seem to be used in this implementation.

}


def _trivial_batch_collator(batch: Any) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

This function does not seem to be used in this implementation.

return batch


def _to_tensor(np_image: np.ndarray) -> torch.Tensor:
Copy link
Member

Choose a reason for hiding this comment

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

This function does not seem to be used in this implementation.

Comment on lines +165 to +168
def _format_detections(
self,
preds
):
Copy link
Member

Choose a reason for hiding this comment

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

This could use a doc-string for what its purpose is in this context, e.g. what preds is expected to be (also needs type annotations).

Comment on lines +174 to +183
for pred in preds:
a_bboxes = []
score_dicts = []
for i, bbox in enumerate(pred):
a_bboxes.append(AxisAlignedBoundingBox(
[bbox[0], bbox[1]], [bbox[2], bbox[3]]))
class_dict = zero_dict.copy()
class_dict[self._classes[i]] = bbox[4]
score_dicts.append(class_dict)
break
Copy link
Member

Choose a reason for hiding this comment

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

Given the single-prediction context of this method compared to Brian's original example, I think these loops should potentially be reworked.

@@ -0,0 +1,275 @@
import logging
Copy link
Member

Choose a reason for hiding this comment

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

This file probably doesn't need to be called "mmdet_base.py": we can drop the "base" part since it doesn't seem that we need to subclass this like the detectron2 impl needed.

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.

2 participants