Conversation
Codecov Report
@@ Coverage Diff @@
## master #184 +/- ##
==========================================
+ Coverage 95.09% 95.32% +0.23%
==========================================
Files 25 26 +1
Lines 1692 1777 +85
==========================================
+ Hits 1609 1694 +85
Misses 83 83
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@zakajd thanks a lot for your contribution! I will have a look tomorrow morning. |
snk4tr
left a comment
snk4tr
left a comment
There was a problem hiding this comment.
According to the codecov, replace_pooling functionality is not covered with tests. Could you add a test for the PieAPPModel that would verify that layers are replaces and model still works after this change?
Overall, this is a great PR! Only minor changes are required.
| self.conv10 = nn.Conv2d(features * 4, features * 8, kernel_size=3, padding=1) | ||
| self.conv11 = nn.Conv2d(features * 8, features * 8, kernel_size=3, padding=1) | ||
|
|
||
| self.fc1_score = nn.Linear(in_features=120832, out_features=512, bias=True) |
There was a problem hiding this comment.
I guess this line makes having features as a parameter senseless because number of input features is hardcoded. I also think that since you use pretrained model weights you can't really change parameters.
There was a problem hiding this comment.
Agree. Removed it from init and hardcoded for now.
As soon as dataset used for PieAPP model training will be released we can reconsider this implementation
There was a problem hiding this comment.
I also added a TODO comment to not forget
piq/pieapp.py
Outdated
| target_weights - prediction_weights | ||
| ) | ||
|
|
||
| # Shape (N, NUM_PATCHES) |
There was a problem hiding this comment.
What is NUM_PATCHES? I don't see this variable in the module
There was a problem hiding this comment.
Removed this comment.
Number of patches depend on input image size, crop size and stride, it's not some fixed value.
piq/pieapp.py
Outdated
| Input images are croped into smaller patches and final score is mean of patch scores. | ||
|
|
||
|
|
||
| Args: | ||
| reduction: Reduction over samples in batch: "mean"|"sum"|"none". |
There was a problem hiding this comment.
If final score is mean of patch scores, then I think reduction parameters is a bit redundant. We either should avoid explicit description in the doctoring (which would be weird if this is a part of an algorithm for the metric) or remove other redundancy options. What do you think?
There was a problem hiding this comment.
Updated docstring description. Final score for individual image is mean of it's patch scores. So we still may want to reduce results over batch.
piq/pieapp.py
Outdated
|
|
||
| x_patches = crop_patches(x, size=64, stride=self.stride) | ||
|
|
||
| features, weights = self.model(x_patches) |
There was a problem hiding this comment.
What do you think about adding this line? May be useful when PieAPP is used as metric.
| features, weights = self.model(x_patches) | |
| with torch.no_grad(): | |
| features, weights = self.model(x_patches) |
There was a problem hiding this comment.
Added torch.autograd.set_grad_enabled context manager and additional flag in init.
Some users may want to use PieAPP as a loss, so turning off all gradients isn't good idea.
piq/pieapp.py
Outdated
| .. [2] https://github.com/prashnani/PerceptualImageError | ||
|
|
||
| """ | ||
| # TODO(zakajd) 10/12/2020: Load weights to release and change this link |
There was a problem hiding this comment.
@snk4tr this PR is ready for merge if you don't have additional comments.
I'm currently in the place with very slow internet, so it would be great if you also upload model weights to last release and change the link in code.
Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>
Signed-off-by: Sergey Kastryulin <snk4tr@gmail.com>
| self.conv10 = nn.Conv2d(features * 4, features * 8, kernel_size=3, padding=1) | ||
| self.conv11 = nn.Conv2d(features * 8, features * 8, kernel_size=3, padding=1) | ||
|
|
||
| self.fc1_score = nn.Linear(in_features=120832, out_features=512, bias=True) |
There was a problem hiding this comment.
I also added a TODO comment to not forget
piq/pieapp.py
Outdated
| .. [2] https://github.com/prashnani/PerceptualImageError | ||
|
|
||
| """ | ||
| # TODO(zakajd) 10/12/2020: Load weights to release and change this link |
|
Kudos, SonarCloud Quality Gate passed!
|
2 participants
Closes #181
Original code is badly written, so I changed it in a few places.
Model structure is similar to perceptual losses, but small differences required to overwrite every call, so I decided not to inherit from
ContentLoss.Proposed Changes