Bug fixes

[ArlindKadra]

Addressed #248

Description:

• Removed wrongly added condition for the starting normalization
• Refactoring the code for the multi-branch options.
• Fixing the implementation for the weighted loss.

[ravinkohli]

I think the residual being added needs to be fixed. Otherwise thanks for the PR, it looks great. Btw, did you test this locally? I remember you saying there was a 15% increase after changing the weighted loss issue. Did you see an increase also for the other changes?

[ArlindKadra]

@ravinkohli I just reverted my "bad" change for the skip connection from the previous PR, that extra change did not seem to affect performance. So, the only thing fixed here is the weighted loss. However, I am testing something small locally since I think there might be another small issue.

[ravinkohli]

@ravinkohli I just reverted my "bad" change for the skip connection from the previous PR, that extra change did not seem to affect performance. So, the only thing fixed here is the weighted loss. However, I am testing something small locally since I think there might be another small issue.

Do you think it's better to add all the bugs to this PR? Or would you prefer merging a PR per fix? I personally prefer the former as we can isolate all the changes in a single commit basically. and in the meantime, we can work off this branch to run experiments.

[ArlindKadra]

@ravinkohli I just reverted my "bad" change for the skip connection from the previous PR, that extra change did not seem to affect performance. So, the only thing fixed here is the weighted loss. However, I am testing something small locally since I think there might be another small issue.

Do you think it's better to add all the bugs to this PR? Or would you prefer merging a PR per fix? I personally prefer the former as we can isolate all the changes in a single commit basically. and in the meantime, we can work off this branch to run experiments.

Yep, I prefer to have the first option. I made the PR so you and Francisco can also have a look at the differences more clearly. I do not want to merge it now.

[ravinkohli]

Hey, all in all, the changes look good. I have left a few suggestions and some questions.

[nabenabe0928]

Thanks for the PR, I will put the comments of the second half of the changes in the github Files changed page.

[nabenabe0928]

Suggested change

	value_range=[1],
	value_range=(1, ),

[ArlindKadra]

to check, if passing a tuple is what is expected.

[ravinkohli]

it expects an Iterable.

[ArlindKadra]

True, but I guess we can be consistent with the other hyperparameter spaces too, so I will do the change.

[nabenabe0928]

Suggested change

	value_range=[1],
	value_range=(1, ),

[nabenabe0928]

Suggested change

	value_range=[1],
	value_range=(1, ),

[nabenabe0928]

Suggested change

	value_range=[1],
	value_range=(1, ),

[nabenabe0928]

Why deleted?

[ArlindKadra]

If I am not mistaken, that is because firstly, we do not add an extra layer here, secondly, this last nonlinearity layer was for the residual block and now it is added there already.

I will double check.

[ArlindKadra]

So, I confirmed it, it is consistent, the last activation layer for the last layer of the last block is in every network_backbone. Something worth mentioning though is that the MLP and ShapedMLP backbone do not have batch norm inside.

[nabenabe0928]

Just to make sure that I have not checked one file.

[nabenabe0928]

Suggested change

	if in_features != out_features:
	if in_features != out_features and self.config["use_skip_connection"]:
	self.shortcut = nn.Linear(in_features, out_features)
	initial_normalization = list()
	if self.config['use_batch_norm']:
	initial_normalization.append(
	nn.BatchNorm1d(in_features)
	)
	initial_normalization.append(
	self.activation()
	)
	self.start_norm = nn.Sequential(
	*initial_normalization
	)

[nabenabe0928]

Suggested change

	return pred
	# The final layer is always softmax now (`pred` already gives pseudo proba)
	return pred

[ArlindKadra]

On another topic, what do you think the else is doing here in this code block @ravinkohli @nabenabe0928, what would be the usage?

[ravinkohli]

its for multilabel multi-class classification. But I don't think we have tested it yet.

[nabenabe0928]

n_threads for the compatibility?

[ArlindKadra]

Sure, done.

[nabenabe0928]

Suggested change

"""

[ArlindKadra]

Why make this change ?

[nabenabe0928]

Suggested change

	https://arxiv.org/abs/1705.07485. The names have been taken from the paper as well.
	Currently, this function supports `even-even`, `shake-even`, `shake-shake` and `M3`.
	"""

[ArlindKadra]

Also this one ?

[nabenabe0928]

n_threads for the compatibility?