pystiche / papers

Reference implementation and replication of prominent NST papers
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

Split sanakoyeu_et_al_2018 transformer and discriminator modules #210

Closed jbueltemeier closed 3 years ago

jbueltemeier commented 3 years ago

Currently all modules of the transformer and the discriminator are stored in two file (modules.py and loss.py), which makes it very confusing to divide between the modules of the transformer and the discriminator. Therefore, here is a suggestion for a better subdivision to make the implementation clearer and thus more comprehensible.

codecov[bot] commented 3 years ago

Codecov Report

Merging #210 into sanakoyeu-2018 will increase coverage by 0.0%. The diff coverage is 97.7%.

Impacted file tree graph

@@              Coverage Diff               @@
##           sanakoyeu-2018    #210   +/-   ##
==============================================
  Coverage            95.4%   95.5%           
==============================================
  Files                  45      47    +2     
  Lines                2115    2135   +20     
==============================================
+ Hits                 2019    2039   +20     
  Misses                 96      96           
Impacted Files Coverage Δ
...stiche_papers/sanakoyeu_et_al_2018/_transformer.py 94.7% <94.7%> (ø)
pystiche_papers/sanakoyeu_et_al_2018/__init__.py 100.0% <100.0%> (ø)
...iche_papers/sanakoyeu_et_al_2018/_discriminator.py 100.0% <100.0%> (ø)
pystiche_papers/sanakoyeu_et_al_2018/_loss.py 100.0% <100.0%> (ø)
pystiche_papers/sanakoyeu_et_al_2018/_modules.py 100.0% <100.0%> (+2.9%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 00652b6...1a6c295. Read the comment docs.

jbueltemeier commented 3 years ago

If I see this right, _loss.py and _modules.py now only contain things that used in the transformer as well as the discriminator, correct?

  1. That's not entirely true. _modules.py contains only modules that occur in both or neither (TransformerBlock) of them. In _loss.py all high-level losses are defined, which will be used as criterion for the transformer or the discriminator, just like in the other implementations.

It might be confusing to have a _loss.py, but actually define all the high-level losses somewhere else.

  1. All high-level losses are defined there. _loss.py contains only the DiscriminatorLoss as criterion for the discriminator, as well as the PerceptualLoss (transformer_loss) and the corresponding losses style_aware_content_loss and tranformed_image_loss.

Maybe move the common thing to _utils.py or _common.py?

  1. _modules.py contain all common components, but also the TransformerBlock, which is not part of the Transformer, but is only used for the transformation of the input images and the output images of the Transformer for another loss. So in my opinion it would not make sense to pack these components in _utils.py or to open _common.py.