use hydra to instantiate loss

[remtav]

No description provided.

[remtav]

Should we name this file ohemce.yaml or keep full name?

[CharlesAuthier]

I agree less long

[remtav]

Ideally, the loss parameter should stay in the training config, but I couldn't point this parameter to the one in config/loss/multiclass use hydra's ${...} syntax. I did try quite a bit. I'd be moving the loss parameter out of training if we can't find a way to make this work.

[remtav]

I really don't like this. CrossEntropy loss (multiclass) is the only loss that uses class_weights as a parameter. We had adapted all other losses to accept it but no use it. The binary losses that @victorlazio109 has added from smp don't accept the "weight" parameter and therefore raise an exception if this parameter is passed.

Logically, class_weights would have been in the crossentropy.yaml config directly (and not in model_choice). It could therefore be dealt with directly by hydra, but one things makes it a bit tricky:
Cross entropy in torch.nn only accepts a tensor. If it could handle a list, this would be much more convenient for hydra.

The only way hydra can directly input a tensor to the weight argument is by changing the CE yaml to somtehing like this:

# @package _global_
loss:
    _target_: torch.nn.CrossEntropyLoss
    ignore_index: ${dataset.ignore_index}
    weight:
        _target_: torch.tensor
        data: ${dataset.class_weights}

This works fine when class_weights is not None (ex.: [0.5,0.5] successfully gets converted to tensor), but when it's None torch.tensor(None) raises an exception. It would require hydra to have some kind of if/else statement if class_weights is None.

Anyways, I'm leaving it to you guys if you have ideas. I've spent enough time on this case. If we don't use CE loss very often and especially if we don't use class_weights much, we could remove the "class_weights" parameter in the dataset config and the yaml could look like this :

# @package _global_
loss:
    _target_: torch.nn.CrossEntropyLoss
    ignore_index: ${dataset.ignore_index}
    # comment out if using class_weights
    # weight:
       #  _target_: torch.tensor
        # data: [0.5, 0.5] 

It's the only alternative I've found, but it is pretty hacky. On the long term, we could raise an issue on torch and ask them to accept a list for class_weights in the CE class.

[remtav]

@victorlazio109 did you run into this "dimensionality" problem when using binary losses? When testing out, I noticed the binary losses only accept logits without the "backgound". Therefore, the prediction array must be indexed as shown in this bit of ugly code from [1,2,256,256] (each number being [batch size, num_classes, width, height]) to [1, 256, 256]. In other words, it becomes necessary to discard the "background" array and squeeze (in the numpy sense of things) the dimensions to match those of the labels [1, 256, 256]

[valhassan]

resolved with my last PR, confirm if you can

[remtav]

I don't think these missing newlines will cause trouble. I can add them if anybody's nervous :)

[valhassan]

where is it passed if removed ?

[remtav]

The only script that is removed is losses/__init__.py. It is replaced by a colleciton of yamls in config/loss directory, one for each loss. Each yaml points to the class that needs to be instantiated by Hydra (ex.: segmentation_models_pytorch.losses.DiceLoss). It's the "hydranic" way of things that will also appear once we tackle issue #246

[remtav]

the get_key_def can't be used as is since the loss will later be instantiated with hydra. The loss parameter is not just a string that can default to "crossentropy"

[valhassan]

We lose the differentiator between SingleClass and MultiClass here, if a user were to pass num_classes == 1 while picking CE loss he will almost certainly run into errors at some point in the code execution

[remtav]

Good point. I can add a small check for this.

[remtav]

@victorlazio109 this is the simplest way I've found to make sure binary losses are not called in multiclass problem and vice versa.