Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Semantic discrepancy between DISTS and ContentLoss parameters #261

Open
snk4tr opened this issue Jul 29, 2021 · 2 comments · May be fixed by #291
Open

Semantic discrepancy between DISTS and ContentLoss parameters #261

snk4tr opened this issue Jul 29, 2021 · 2 comments · May be fixed by #291
Assignees
Labels
bug Something isn't working

Comments

@snk4tr
Copy link
Contributor

snk4tr commented Jul 29, 2021

Describe the bug
ContentLoss is a super class of multiple feature-based metrics including DISTS. However, there is a semantic discrepancy between these two for parameters such as weights and layers:

  • For ContentLoss: weights is a collection of scalars, which are used to scale feature outputs from layers;
  • For DISTS: weights is a collection of scalars, which are used to scale features that are computed in a sophisticated way, provided by the custom self.distance function, which produces two types of features (structure_distance and texture_distance), which also include the initial data tensor.

This misalignment leads to the following: in general case len(weights) == len(layers) because otherwise some weights or layers are ignores and not used during computation of the feature-based metric. However, neither assert nor warning can be used in the ContentLoss class because it will provide incorrect behaviour for DISTS.

To Reproduce
Steps to reproduce the behavior:

  1. Put assert len(layers) == len(weights) in the initializer for the ContentLoss class;
  2. Run tests or manually run code with feature-based metrics;
  3. Observe that initialization of DISTS fails due to the added assert;
  4. Observe error message, where len(layers) == 5 and len(weights) == 12.

Expected behavior
All feature-based metrics have the same semantic for passed parameters.

Additional context
The problem was revealed during work on #258

@snk4tr snk4tr added the bug Something isn't working label Jul 29, 2021
@zakajd
Copy link
Collaborator

zakajd commented Jul 29, 2021

I agree that current DISTS implementation is suboptimal. One possible solution is implement it as a separate class and move to another file

@zakajd
Copy link
Collaborator

zakajd commented Aug 5, 2021

Discussed during monthly meeting that we need to refactor code for perceptual metrics and split them to work individually without complicated inheritance.

@zakajd is responsible for this

@zakajd zakajd linked a pull request Feb 6, 2022 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants