fixed output_list | now it has only 5 elements instead of 5*batchsize#2914
Merged
vfdev-5 merged 4 commits intopytorch:masterfrom Apr 14, 2023
Merged
fixed output_list | now it has only 5 elements instead of 5*batchsize#2914vfdev-5 merged 4 commits intopytorch:masterfrom
vfdev-5 merged 4 commits intopytorch:masterfrom
Conversation
vfdev-5
reviewed
Apr 14, 2023
Collaborator
|
@moienr thanks for the explanation and the PR! Right now I see the problem, it should be basically : output_list = [outputs[x * batch_size : (x + 1) * batch_size] for x in range(5)]where 5 refers to the size of I made a suggestion how to fix code style and introduced batch size variable. Please update your PR and make sure to run code style auto formatting to avoid formatting issues: |
batch_size variable instead of using y_pred.size(0) directly Co-authored-by: vfdev <[email protected]>
Contributor
Author
|
Thank you dear @vfdev-5 I committed your code, and ran the style. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2913
Description:
This pull request updates the code in
ignite/metrics/ssim.pyso it won't create extratorch.Tensors.The changes involve modifying the list comprehension that creates the
output_list, which has the wrong length ofBatchSize*5instead of it being5(the same aslen([y_pred, y, y_pred * y_pred, y * y, y_pred * y])). The elements with index>4 are empty tensors with shape(0,C,H1,W1)in theoutput_listInstead of concatenating the tensor inputs in
torch.cat([y_pred, y, y_pred * y_pred, y * y, y_pred * y]), I have created a list of the input tensors asinput_list= [y_pred, y, y_pred * y_pred, y * y, y_pred * y], which I believe was the actual intention of the author.Now we have access to the Length of this list, which we will use to retrieve
input_listelements after passing them through aconv2dlayer.I then passed the concatenated input tensor using
torch.cat(input_list)which has a size of(B*5, C, H1, W1)to theF.conv2dfunction call.After that, the
outputswhich is a tensor with shape(B*5, C, H, W)is sliced for everyBatchelement to return a list of5elements. this is where the previous code had a problem and instead of a list of5(orlen(input_list)) tensors, it was returning a list ofB*5elements, where only its first five elements were actual tensors and the rest where empty when theBatch_size>1.(the previous code was calling
tesnor[index1:index2]whereindex1andindex2were larger thantensor.size(0)which was returning empty tensors)We can see in lines
167-173that the elements with indices0-4are being used, which proves this issue.I have tested these changes on my local machine and confirmed that they do not introduce any new issues. Please let me know if there are any concerns or feedback regarding these changes.
Thanks for considering this pull request!
Check list: