Distributed training [SemSeg]

[sanskar107]

This change is 

[lgtm-com]

This pull request introduces 31 alerts and fixes 6 when merging e4c4441 into c461790 - view on LGTM.com

new alerts:

• 19 for Unused import
• 4 for Unused local variable
• 2 for Signature mismatch in overriding method
• 2 for First parameter of a method is not named 'self'
• 2 for Unreachable code
• 1 for Use of the return value of a procedure
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 4 for Unused local variable
• 2 for Unused import

[lgtm-com]

This pull request introduces 45 alerts and fixes 6 when merging 4ff5fbe into c461790 - view on LGTM.com

new alerts:

• 28 for Unused import
• 5 for Unused local variable
• 4 for Signature mismatch in overriding method
• 4 for First parameter of a method is not named 'self'
• 2 for Unreachable code
• 1 for Use of the return value of a procedure
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 4 for Unused local variable
• 2 for Unused import

[lgtm-com]

This pull request introduces 45 alerts and fixes 6 when merging 1803d77 into c461790 - view on LGTM.com

new alerts:

• 28 for Unused import
• 5 for Unused local variable
• 4 for Signature mismatch in overriding method
• 4 for First parameter of a method is not named 'self'
• 2 for Unreachable code
• 1 for Use of the return value of a procedure
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 4 for Unused local variable
• 2 for Unused import

[lgtm-com]

This pull request introduces 45 alerts and fixes 6 when merging c8f4589 into c461790 - view on LGTM.com

new alerts:

• 28 for Unused import
• 5 for Unused local variable
• 4 for Signature mismatch in overriding method
• 4 for First parameter of a method is not named 'self'
• 2 for Unreachable code
• 1 for Use of the return value of a procedure
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 4 for Unused local variable
• 2 for Unused import

[lgtm-com]

This pull request introduces 44 alerts when merging 40f888f into 1c45bfe - view on LGTM.com

new alerts:

• 26 for Unused import
• 8 for Unused local variable
• 4 for Signature mismatch in overriding method
• 4 for First parameter of a method is not named 'self'
• 1 for Unreachable code
• 1 for Module is imported with 'import' and 'import from'

[lgtm-com]

This pull request introduces 33 alerts and fixes 1 when merging 375bb8f into 1c45bfe - view on LGTM.com

new alerts:

• 20 for Unused import
• 4 for Signature mismatch in overriding method
• 4 for First parameter of a method is not named 'self'
• 3 for Unused local variable
• 1 for Unreachable code
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 33 alerts and fixes 1 when merging 50c8c20 into 1c45bfe - view on LGTM.com

new alerts:

• 20 for Unused import
• 4 for Signature mismatch in overriding method
• 4 for First parameter of a method is not named 'self'
• 3 for Unused local variable
• 1 for Unreachable code
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 8 alerts and fixes 4 when merging 5c2ba32 into 1c45bfe - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Signature mismatch in overriding method
• 1 for Unused local variable
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 3 for Unused import
• 1 for Module is imported with 'import' and 'import from'

[lgtm-com]

This pull request introduces 8 alerts and fixes 4 when merging e971747 into 1c45bfe - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Signature mismatch in overriding method
• 1 for Unused local variable
• 1 for Module is imported with 'import' and 'import from'

fixed alerts:

• 3 for Unused import
• 1 for Module is imported with 'import' and 'import from'

[benjaminum]

Reviewed 18 of 23 files at r1.
Reviewable status: 18 of 23 files reviewed, 4 unresolved discussions (waiting on @sanskar107 and @ssheorey)

---

ml3d/configs/sparseconvunet_scannet.yml line 9 at r1 (raw file):

  use_cache: False
  sampler:
    name: None

Why change from 'SemsegRandomSampler' to None?

---

ml3d/configs/default_cfgs/kitti360.yml line 2 at r1 (raw file):

name: KITTI360
dataset_path: /Users/sanskara/data/kitti360/

hardcoded path

---

ml3d/torch/models/sparseconvnet.py line 119 at r1 (raw file):

        feat = np.array(data['feat'], dtype=np.float32)
        if feat.shape[1] < 3:
            feat = np.concatenate([feat, np.ones([feat.shape[0], 1])], 1)

Why force the feat size to be >= 3?

---

ml3d/torch/modules/metrics/semseg_metric.py line 30 at r1 (raw file):

    def __iadd__(self, otherMetric):
        if self.confusion_matrix is None and otherMetric.confusion_matrix is None:
            pass

Why not throw an exception?

[ssheorey]

Reviewable status: 18 of 23 files reviewed, 22 unresolved discussions (waiting on @benjaminum, @sanskar107, and @ssheorey)

---

ml3d/datasets/kitti360.py line 68 at r1 (raw file):

            raise ValueError(
                "Invalid Path, make sure dataset_path is the parent directory of data_3d_semantics."
            )

[nit] catch error from open and throw informative message instead of a separate check.

---

ml3d/datasets/kitti360.py line 160 at r1 (raw file):

            attr: The attributes that correspond to the outputs passed in results.
    """

move docstring after function signature.

---

ml3d/datasets/nuscenes_semseg.py line 63 at r1 (raw file):

        self.val_info = {}

        if os.path.exists(join(info_path, 'infos_train.pkl')):

open() will throw an error if path is not readable - no need to separately check exists().
Also for the 2 following files.

---

ml3d/datasets/nuscenes_semseg.py line 111 at r1 (raw file):

        }
        self.label_mapping = np.array(
            [mapping[i] for i in range(0, len(mapping))], dtype=np.int32)

np.fromiter(mapping.values(), dtype=np.int32) should be the same

---

ml3d/datasets/nuscenes_semseg.py line 122 at r1 (raw file):

        """
        classes = "ignore, barrier, bicycle, bus, car, construction_vehicle, motorcycle, pedestrian, traffic_cone, trailer, trucl, driveable_surface, other_flat, sidewalk, terrain, manmade, vegetation"
        classes = classes.replace(', ', ',').split(',')

can directly split(', ')

---

ml3d/datasets/nuscenes_semseg.py line 136 at r1 (raw file):

            A data object with lidar information.
        """
        assert Path(path).exists()

assert not required.

---

ml3d/datasets/nuscenes_semseg.py line 146 at r1 (raw file):

            A data object with semantic information.
        """
        assert Path(path).exists()

assert not required

---

ml3d/datasets/nuscenes_semseg.py line 199 at r1 (raw file):

                attribute is stored; else, returns false.
        """
        pass

raise NotImplementedError() instead of pass to prevent silent bugs

---

ml3d/datasets/nuscenes_semseg.py line 210 at r1 (raw file):

                results.
        """
        pass

raise NotImplementedError() instead of pass to prevent silent bugs

---

ml3d/datasets/waymo_semseg.py line 81 at r1 (raw file):

        """
        classes = "Undefined, Car, Truck, Bus, Other Vehicle, Motorcyclist, Bicyclist, Pedestrian, Sign, Traffic Light, Pole, Construction Cone, Bicycle, Motorcycle, Building, Vegetation, Tree Trunk, Curb, Road, Lane Marker, Other Ground, Walkable, Sidewalk"
        classes = classes.replace(', ', ',').split(',')

can directly split(", ") without replace()

---

ml3d/torch/pipelines/semantic_segmentation.py line 164 at r1 (raw file):

            for unused_step, inputs in enumerate(infer_loader):
                if hasattr(inputs['data'], 'to'):
                    inputs['data'].to(device)

Isn't inputs['data'] always a torch tensor, so it should always have ato() function?

---

ml3d/torch/pipelines/semantic_segmentation.py line 364 at r1 (raw file):

            if train_sampler is not None or valid_sampler is not None:
                raise NotImplementedError(
                    "Distributed training with sampler is not supported yet!")

Collect any limitations and future TODOs such as this in the PR description.

---

scripts/preprocess_waymo.py line 69 at r1 (raw file):

    def __init__(self, dataset_path, save_dir='', workers=8, is_test=False):

        self.write_image = True

Remove if from debugging

---

scripts/preprocess_waymo.py line 138 at r1 (raw file):

                    frame.context.stats.location
                    not in self.selected_waymo_locations):
                print("continue")

Is this from debugging? Remove if so.

---

scripts/preprocess_waymo.py line 211 at r1 (raw file):

                'w+') as fp_calib:
            fp_calib.write(calib_context)
            fp_calib.close()

close() is redundant with context manager open()

---

scripts/preprocess_waymo.py line 260 at r1 (raw file):

            # if self.filter_empty_3dboxes and obj.num_lidar_points_in_box < 1:
            #     continue

remove commented code.

---

scripts/preprocess_waymo.py line 272 at r1 (raw file):

            # pt_ref = self.T_velo_to_front_cam @ \
            #     np.array([x, y, z, 1]).reshape((4, 1))
            # x, y, z, _ = pt_ref.flatten().tolist()

remove commented code

---

scripts/preprocess_waymo_semseg.py line 41 at r1 (raw file):

    parser.add_argument(
        '--out_path',
        help='Output path to store pickle (default to dataet_path)',

typo