Future PR: Add license header to all files, and add check for license header to CI. See main repo.

Reviewed 8 of 13 files at r1, 3 of 5 files at r2.
Reviewable status: 11 of 15 files reviewed, 9 unresolved discussions (waiting on @cosama, @sanskar107, and @ssheorey)

ml3d/configs/randlanet_parislille3d.yml, line 40 at r2 (raw file):

  adam_lr: 0.01
  batch_size: 2
  learning_rate: 0.01

Use new hierarchical config format (optimizer -> lr) like in SparseConvUNet
Are adam_lr and learning_rate duplicates?

ml3d/configs/randlanet_parislille3d.yml, line 42 at r2 (raw file):

  learning_rate: 0.01
  main_log_dir: ./logs
  max_epoch: 100

Can you confirm that 100 epochs are enough now instead of 200?

ml3d/configs/randlanet_parislille3d.yml, line 54 at r2 (raw file):

    noise_level: 0.01
    min_s: 0.9
    max_s: 1.1

Are we reducing augmentation?

ml3d/configs/randlanet_s3dis.yml, line 38 at r2 (raw file):

pipeline:
  name: SemanticSegmentation
  adam_lr: 0.01

optimizer -> lr

ml3d/configs/randlanet_semantic3d.yml, line 52 at r2 (raw file):

  max_epoch: 100
  save_ckpt_freq: 5
  scheduler_gamma: 0.9886

nit: Is this hand-tuned? Would 0.99 work just as well (it's simpler)?

ml3d/datasets/augment/augmentation.py, line 23 at r2 (raw file):

        Args:
            data: Pointcloud or features.
            cfg: configuration dictionary.

Document config option used. e.g.: Key 'dim' specifies...

ml3d/datasets/augment/augmentation.py, line 75 at r2 (raw file):

        """
        if np.abs(pc[:, :2].mean()) > 1e-2:

Why should we skip the Z dimension check?

tests/test_models.py, line 60 at r2 (raw file):

    out = net(inputs).detach().numpy()

    assert out.shape == (1, 5000, 10)

[Future PR OK] Can we verify the output values as well? This will guard against accidental changes to the code that actually affect the model. One option to do that is:

• Create new RNG with fixed seed and create inputs and weights from this RNG.
• Scale inference output to int32 range and quantize to int32.
• Calculate hash: https://stackoverflow.com/a/66376073 and compare with hash value stored in test.

Reviewable status: 11 of 15 files reviewed, 31 unresolved discussions (waiting on @cosama, @sanskar107, and @ssheorey)

ml3d/tf/models/randlanet.py, line 18 at r2 (raw file):

    Based on the architecture
    https://arxiv.org/abs/1911.11236#
    """

Keep link to reference implementation.
Also, more details on this model would be useful here. e.g.: image of the architecture and a brief description. Perhaps copying the paper abstract can be a good overview.

To add an image, drag and drop the image to the PR description box and add the image URL with rst image directive. The image will show up in sphinx.

ml3d/tf/models/randlanet.py, line 61 at r2 (raw file):

                                                      momentum=0.99,
                                                      epsilon=1e-6)
        self.lr0 = tf.keras.layers.LeakyReLU(0.2)

Can we group these 3 together? Perhaps add to encoder, or as separate input layer.

Code quote:

        self.fc0 = tf.keras.layers.Dense(cfg.dim_features)
        self.bn0 = tf.keras.layers.BatchNormalization(-1,
                                                      momentum=0.99,
                                                      epsilon=1e-6)
        self.lr0 = tf.keras.layers.LeakyReLU(0.2)

ml3d/tf/models/randlanet.py, line 76 at r2 (raw file):

            encoder_dim_list.append(dim_feature)

        # self.encoder = tf.keras.models.Sequential(self.encoder)

Remove commented code

ml3d/tf/models/randlanet.py, line 92 at r2 (raw file):

            dim_feature = encoder_dim_list[-i - 2]

        # self.decoder = tf.keras.models.Sequential(self.decoder)

remove commented code

ml3d/tf/models/randlanet.py, line 160 at r2 (raw file):

        Args:
            outputs: logits
            labels: labels

Update docstring to match function arguments. Add expected shape.

ml3d/tf/models/randlanet.py, line 285 at r2 (raw file):

                    feat = np.concatenate([pc, feat], axis=1)

                assert cfg.in_channels == feat.shape[

assert -> raise RuntimeError

ml3d/tf/models/randlanet.py, line 326 at r2 (raw file):

            feat = np.concatenate([pc, feat], axis=1)

        assert self.cfg.in_channels == feat.shape[

assert -> raise RuntimeError

ml3d/tf/models/randlanet.py, line 392 at r2 (raw file):

        # input_list += [feat, label]

        # return input_list

Remove commented code

Code quote:

        # input_list = input_points + input_neighbors + input_pools + input_up_samples
        # input_list += [feat, label]

        # return input_list

ml3d/tf/models/randlanet.py, line 551 at r2 (raw file):

        self.encode_pos = encode_pos

        # self.device = device

remove commented code

ml3d/tf/models/randlanet.py, line 589 at r2 (raw file):

                tf.Tensor of shape (B, d, N, 1)
            neighbor_indices: indices of k neighbours.
                tf.Tensor of shape (B, N, K)

Add docs for training and relative_features

ml3d/torch/models/randlanet.py, line 22 at r2 (raw file):

    Based on the architecture
    https://arxiv.org/abs/1911.11236#
    """

[Same as for the TF version] keep ref. implementation link and add description + architecture image from paper.

ml3d/torch/models/randlanet.py, line 63 at r2 (raw file):

        self.fc0 = nn.Linear(cfg.in_channels, cfg.dim_features)
        self.bn0 = nn.BatchNorm2d(cfg.dim_features, eps=1e-6, momentum=0.01)

[save as TF version] group into one layer if possible.

ml3d/torch/models/randlanet.py, line 177 at r2 (raw file):

        if 'recenter' in augment_cfg:
            val_augment_cfg['recenter'] = augment_cfg['recenter']
            augment_cfg.pop('recenter')

val_augment_cfg['recenter'] = augment_cfg.pop('recenter')

Code quote:

            val_augment_cfg['recenter'] = augment_cfg['recenter']
            augment_cfg.pop('recenter')

ml3d/torch/models/randlanet.py, line 180 at r2 (raw file):

        if 'normalize' in augment_cfg:
            val_augment_cfg['normalize'] = augment_cfg['normalize']
            augment_cfg.pop('normalize')

same^^^

ml3d/torch/models/randlanet.py, line 196 at r2 (raw file):

            feat = np.concatenate([pc, feat], axis=1)

        assert cfg.in_channels == feat.shape[

assert -> raise

ml3d/torch/models/randlanet.py, line 351 at r2 (raw file):

        Args:
            outputs: logits
            labels: labels

update docstring with shape and other args (+description as appropriate)

ml3d/torch/models/randlanet.py, line 426 at r2 (raw file):

            return False

    def update_probs(self, inputs, results, test_probs, test_labels):

Add docstring.

ml3d/torch/models/randlanet.py, line 447 at r2 (raw file):

class SharedMLP(nn.Module):

Add docstring: purpose of this class

ml3d/torch/models/randlanet.py, line 494 at r2 (raw file):

class LocalSpatialEncoding(nn.Module):

docstring: purpose of class

ml3d/torch/models/randlanet.py, line 503 at r2 (raw file):

        self.encode_pos = encode_pos

        # self.device = device

remove commented code

ml3d/torch/models/randlanet.py, line 578 at r2 (raw file):

class AttentivePooling(nn.Module):

docstring: purpose of class.

ml3d/torch/models/randlanet.py, line 609 at r2 (raw file):

class LocalFeatureAggregation(nn.Module):

docstring: purpose of class

ml3d/torch/models/randlanet.py, line 644 at r2 (raw file):

        """
        # knn_output = knn(coords.cpu().contiguous(), coords.cpu().contiguous(), self.num_neighbors)

remove commented code.

Reviewable status: 5 of 15 files reviewed, 31 unresolved discussions (waiting on @cosama and @ssheorey)

ml3d/configs/randlanet_parislille3d.yml, line 40 at r2 (raw file):

Done. Removed the duplicates.

ml3d/configs/randlanet_parislille3d.yml, line 42 at r2 (raw file):

Actually the training never converged on ParisLille3d. Not now, and not the last time. We converted the author's weights. But usually 100 epochs should suffice.

ml3d/configs/randlanet_parislille3d.yml, line 54 at r2 (raw file):

Here turn_on is false, so none of the augmentations were being used. I tried our some from our new augmentation class, but that didn't help much. They tend to marginally lower the performance.

ml3d/configs/randlanet_s3dis.yml, line 38 at r2 (raw file):

Done.

ml3d/configs/randlanet_semantic3d.yml, line 52 at r2 (raw file):

We picked this value from author's code, and I believe this might be hand tuned.
After 100 epochs, 0.99 will make learning rate 36% of initial value, while 0.9886 will make 30%. It shouldn't make any difference. Should I change it to 0.99 ?

ml3d/datasets/augment/augmentation.py, line 23 at r2 (raw file):

Done.

ml3d/datasets/augment/augmentation.py, line 75 at r2 (raw file):

It sometime helps to preserve the absolute height dimension. Usually height value is not very high and doesn't affect much if we rotate without recentering.

ml3d/tf/models/randlanet.py, line 18 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 61 at r2 (raw file):

We can group them, I kept it similar to author's code. But is it critical ? The problem is that it will change name in the model state dict. Either we have to train on all datasets again, or manually strip down weights file and change the layer name.

ml3d/tf/models/randlanet.py, line 76 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 92 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 160 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 285 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 326 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 392 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 551 at r2 (raw file):

Done.

ml3d/tf/models/randlanet.py, line 589 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 22 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 63 at r2 (raw file):

Same comment as TF version

ml3d/torch/models/randlanet.py, line 177 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 180 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 196 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 351 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 426 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 447 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 494 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 503 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 578 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 609 at r2 (raw file):

Done.

ml3d/torch/models/randlanet.py, line 644 at r2 (raw file):

Done.

tests/test_models.py, line 60 at r2 (raw file):

Thanks for the reviews. This makes sense. I'll add these tests to verify the output in a future PR, along with license header for each file.

Reviewed 2 of 13 files at r1, 2 of 5 files at r2.
Reviewable status: 7 of 15 files reviewed, 1 unresolved discussion (waiting on @sanskar107 and @ssheorey)

ml3d/configs/randlanet_parislille3d.yml, line 42 at r2 (raw file):

OK. We should mention this in the main readme table (with a (*) Using weights from original author, for example). Users may likely run into this reproducibility issue as well.

ml3d/configs/randlanet_parislille3d.yml, line 54 at r2 (raw file):

OK. Perhaps then change to dim: [] Is turn_on a default key? I don't see it in this file.

ml3d/configs/randlanet_semantic3d.yml, line 52 at r2 (raw file):

Don't worry about it - it will affect the weights. Going with the author's settings sounds good.

ml3d/datasets/augment/augmentation.py, line 75 at r2 (raw file):

Good idea. Can you add this as a comment?

ml3d/tf/models/randlanet.py, line 61 at r2 (raw file):

OK, not critical.

Reviewable status: 6 of 15 files reviewed, 1 unresolved discussion (waiting on @ssheorey)

ml3d/configs/randlanet_parislille3d.yml, line 42 at r2 (raw file):

Done

ml3d/configs/randlanet_parislille3d.yml, line 54 at r2 (raw file):

turn_on was an ON/OFF check used in old augmentation methods. Not used anymore.

ml3d/datasets/augment/augmentation.py, line 75 at r2 (raw file):

Done.