Remove logsoftmax

[Sara04]

Addressing the issue #511 by setting up the flag add_logsoftmax with a warning that the LogSoftmax as a final layer will be removed in the future. Proposed by @PierreGtch . Cf #457

[sliwy]

Thx @Sara04 Great job! I like the idea of convention of having out_fun and the approach of LogSoftmax/Identity. Some details I want to discuss:

1. I have doubts whether property called add_log_softmax is not misleading, suggesting that there is an action of adding log_softmax being performed. I have commented with some propositions (@bruAristimunha, @PierreGtch please tell what you think as well).
2. If I understand well this PR we need to add add_log_softmax param in all the models as a parameter and then use it in the super.__init__(). In most of the models it is always set to True in super.__init__(), I left comments for some of the cases here.

Edit: I didn't notice that it is a draft, so maybe reviewed too early 😂

[Sara04]

Thank you very much for your comments, @sliwy ! 😄
I will reply to them one by one.

[PierreGtch]

Hi @sliwy, thanks for your review! We discuss your comments with Sara and I will answer them individually

[sliwy]

@Sara04 @PierreGtch based on this #513 (comment) I think that I may not understand well what the flow of logsoftmax is going to look like. For now, all models (or almost all) are created with softmax in the end. In the new flow if a user wants to have classification model would need to add softmax on their own?

[codecov]

Codecov Report

Merging #513 (9f24993) into master (6f213a8) will decrease coverage by 0.05%.
The diff coverage is 90.90%.

❗ Current head 9f24993 differs from pull request most recent head be3455f. Consider uploading reports for the commit be3455f to get more accurate results

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
- Coverage   84.37%   84.32%   -0.05%     
==========================================
  Files          63       63              
  Lines        4545     4569      +24     
==========================================
+ Hits         3835     3853      +18     
- Misses        710      716       +6     

[PierreGtch]

@Sara04 @PierreGtch based on this #513 (comment) I think that I may not understand well what the flow of logsoftmax is going to look like. For now, all models (or almost all) are created with softmax in the end. In the new flow if a user wants to have classification model would need to add softmax on their own?

Yes exactly, the user will have to add the softmax himself as this is standard practice. But in practice, you will not need to because if you use the nn.CrossEntropyLoss, it’s already integrated in the loss.

[sliwy]

ok, cool, sounds awesome!

[bruAristimunha]

Can we merge this @Sara04, @PierreGtch and @sliwy? Looks okay for me.

[PierreGtch]

Just two small changes in case the add_log_softmax was already there before and defaulted to false: we should keep the same default.

Otherwise all good to merge! Thanks a lot @Sara04 for all this work!!!

changes were made

[bruAristimunha]

Thank you @Sara04, @PierreGtch, @sliwy!

[Sara04]

Thanks @PierreGtch , @sliwy , @bruAristimunha !