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

Add the example of super_resolution #2885

Merged
merged 19 commits into from
Mar 21, 2023
Merged

Conversation

guptaaryan16
Copy link
Contributor

@guptaaryan16 guptaaryan16 commented Mar 3, 2023

Related to #2878

Description: I have added the example for the super resolution in the PyTorch/examples

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the examples Examples label Mar 3, 2023
@guptaaryan16
Copy link
Contributor Author

Hey @vfdev-5 , thanks for assigning the issue and since I had my mid-terms I could not work on this earlier so I apologise for the delay

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @guptaaryan16
I left few comments.
Can you share execution logs, how much time it takes to run one epoch ?
Were you be able to train a simple SR model and share inference example ?

examples/super_resolution/README.md Outdated Show resolved Hide resolved
examples/super_resolution/README.md Outdated Show resolved Hide resolved
examples/super_resolution/data.py Outdated Show resolved Hide resolved
examples/super_resolution/main.py Outdated Show resolved Hide resolved

def __getitem__(self, index):
input = load_img(self.image_filenames[index])
target = input.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like in this application we are using input image as target, so we can replace BSD data by another built-in torchvision dataset instead of manually code how to download/extract the data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it will be better (it was implemented this way in pytorch/examples)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it is working well as shown in Colab notebook

@guptaaryan16
Copy link
Contributor Author

I think this colab notebook will be able to cover this example
https://colab.research.google.com/drive/1mg6TzaUOgy8CJ7TwGVOKHNZJ1UhE6u9E?usp=sharing

@guptaaryan16
Copy link
Contributor Author

A lot of things covered in this example were directly present in PyTorch/examples repo of the same example so it may contain some similar errors

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 6, 2023

@guptaaryan16 code in pytorch/examples seems a bit outdated and rather bulky. For ignite, I think it would worth to provide up to date code and with interesting outcome.

First question about the results from the colab, how super-resolved image differs from an image upsampled with bicubic mode ? Maybe, it worth to include PSNR for bicubic upsampled image for reference when doing validation stage.
It would be also funny to super-resolve for test images those from cifar10.

@guptaaryan16
Copy link
Contributor Author

Hey @vfdev-5 I have made suggested changes in the colab notebook. Also I was exploring the link https://github.com/Coloquinte/torchSR and I think it will be better to adapt a train script from here rather than PyTorch/examples. Can you give your suggestion on this?

@guptaaryan16
Copy link
Contributor Author

Hey @vfdev-5 I have made all suggested changes including adding the torchvision dataset. Please review

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @guptaaryan16

I left few comments in the code. I think we can keep this model and the training script but change the training dataset and use additionally CIFAR10 for inference only.

examples/super_resolution/main.py Outdated Show resolved Hide resolved
examples/super_resolution/main.py Outdated Show resolved Hide resolved
examples/super_resolution/main.py Outdated Show resolved Hide resolved
@guptaaryan16
Copy link
Contributor Author

Hey @vfdev-5 please review

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @guptaaryan16
I left few more comments concerning the code.
Please try to train a model and share training logs and run super_resolve script on a cifar10 image then compare it with the same image upsampled with bicubic interp mode.

Comment on lines 60 to 61
dim = self.crop_size // self.scale_factor
lr_image = resize(lr_image, [dim, dim])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dim = self.crop_size // self.scale_factor
lr_image = resize(lr_image, [dim, dim])
size = self.crop_size // self.scale_factor
lr_image = resize(lr_image, [size, size])


training_data_loader = DataLoader(dataset=trainset_sr, num_workers=opt.threads, batch_size=opt.batch_size, shuffle=True)
testing_data_loader = DataLoader(
dataset=testset_sr, num_workers=opt.threads, batch_size=opt.test_batch_size, shuffle=False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dataset=testset_sr, num_workers=opt.threads, batch_size=opt.test_batch_size, shuffle=False
dataset=testset_sr, num_workers=opt.threads, batch_size=opt.test_batch_size

examples/super_resolution/main.py Show resolved Hide resolved
model = model.cuda()
input = input.cuda()

out = model(input)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
out = model(input)
model.eval()
with torch.no_grad():
out = model(input)

@guptaaryan16
Copy link
Contributor Author

Hey @vfdev-5 I have updated the colab link with the necessary training logs and updates along with the cifar-10 example
https://colab.research.google.com/drive/1mg6TzaUOgy8CJ7TwGVOKHNZJ1UhE6u9E?usp=sharing

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @guptaaryan16
I approve this PR. Next step would be to integrate it into the CI and use other ignite tools like progress bar to show sort of best practices

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants