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

fix 2 problems in tool/preprocess_reds_dataset.py #148

Merged
merged 24 commits into from
Oct 26, 2020
Merged

fix 2 problems in tool/preprocess_reds_dataset.py #148

merged 24 commits into from
Oct 26, 2020

Conversation

wwhio
Copy link
Contributor

@wwhio wwhio commented Oct 9, 2020

fix 2 problems in tool/preprocess_reds_dataset.py

  1. root-path in argparse
  2. unzipped data in wrong path structure

wwhio added 3 commits October 9, 2020 15:37
1.Change argparse to correctly parse `root-path`

2.Change unzip folder to correct path:

original:
unzip ./data/REDS/train_sharp.zip to ./data/REDS/./data/REDS/train_sharp

now:
unzip ./data/REDS/train_sharp.zip to ./data/REDS/train_sharp
change document accordingly 
`root-path` in preprocess_reds_dataset.py
fix `root-path` argument, fix unziped data structure
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #148 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #148   +/-   ##
=======================================
  Coverage   82.25%   82.25%           
=======================================
  Files         145      145           
  Lines        6711     6711           
  Branches      994      994           
=======================================
  Hits         5520     5520           
  Misses       1090     1090           
  Partials      101      101           
Flag Coverage Δ
#unittests 82.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 81b8464...052c397. Read the comment docs.

@hellock hellock requested a review from nbei October 10, 2020 07:42
@wwhio
Copy link
Contributor Author

wwhio commented Oct 10, 2020

One more problem when training : cannot find annotation file.

please change line 137 in tools/preprocess_reds_dataset.py from

def generate_anno_file(root_path, file_name='REDS/meta_info_REDS_GT.txt'):

to

def generate_anno_file(root_path, file_name='meta_info_REDS_GT.txt'):

Since root_path is ./data/REDS, file_name do not need REDS/ prefix anymore.
Sorry for I don't create a new PR for this minor fix.

wwhio added 4 commits October 10, 2020 20:29
`tensoboard` is used by training edvr
since `root_path` is `./data/REDS`, remove `REDS` from generate_anno_file `file_name`
@wwhio
Copy link
Contributor Author

wwhio commented Oct 10, 2020

fix some problems when I try to train edvr

@nbei
Copy link
Collaborator

nbei commented Oct 11, 2020

Hi @wwhio, could you please fix these errors from linting?

configs/restorers/edvr/edvrm_x4_g8_600k_reds.py:137:70: E261 at least two spaces before inline comment
configs/restorers/edvr/edvrm_x4_g8_600k_reds.py:137:80: E501 line too long (123 > 79 characters)
tools/preprocess_reds_dataset.py:178:49: E231 missing whitespace after ','
configs/restorers/edvr/edvrm_wotsa_x4_g8_600k_reds.py:137:70: E261 at least two spaces before inline comment
configs/restorers/edvr/edvrm_wotsa_x4_g8_600k_reds.py:137:80: E501 line too long (123 > 79 characters)

@wwhio
Copy link
Contributor Author

wwhio commented Oct 12, 2020

Hi @nbei, could you tell me how to do linting test locally ?
How can I get the output of these linting errors ?
I have try to fix these linting errors, but these errors still appear in the last check.

for linting
@nbei
Copy link
Collaborator

nbei commented Oct 12, 2020

Hi @wwhio , you can use pre-commit hook to check these error and correct them.

@@ -83,7 +83,7 @@ def load_annotations(self):
dict(
lq_path=self.lq_folder,
gt_path=self.gt_folder,
key=key,
key=key.split('(')[0].strip(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the key will not contain '('. What is the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key is loaded from annotation file, which is 000_00000000.png (720,1280,3) for example.
But we need key=000_00000000.png.

The annotation file generate function:
https://github.com/wwhio/mmediting/blob/9a93413a10cf208d7a08d4db0e7937c6adae6a2d/tools/preprocess_reds_dataset.py#L137

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I check this again. A line in anno_file is 259/00000053 (720, 1280, 3), and key is 259/00000053 (720, 1280, 3) before modification.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I think a better way is to modify:

https://github.com/open-mmlab/mmediting/blob/9a93413a10cf208d7a08d4db0e7937c6adae6a2d/mmedit/datasets/sr_reds_dataset.py#L61-L64

maybe keys = [v.strip().split('.')[0] for v in fin] should be keys = [v.strip().split(' ')[0] for v in fin]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@wwhio wwhio Oct 12, 2020

Choose a reason for hiding this comment

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

I follow your first suggestion to modify mmediting/mmedit/dataset/sr_reds_dataset.py to keep the modification small and keep consistent with vimeo90k dataset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I roll back changes to mmediting/mmedit/dataset/sr_reds_dataset.py and some related files. The real problem lies in the generated annotation file.

@wwhio wwhio requested a review from xinntao October 14, 2020 13:25
@xinntao
Copy link
Collaborator

xinntao commented Oct 20, 2020

It looks good to me~

data_folder = osp.join(unzip_folder, data_type, data_name)
for i in os.listdir(data_folder):
shutil.move(osp.join(data_folder, i), unzip_folder)
shutil.rmtree(osp.join(unzip_folder, data_type))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

please move this line under if osp.isdir(osp.join(unzip_folder, data_type, data_name)):

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since I have not merged this PR, could you just modify it so that I can merge it in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@nbei nbei merged commit 941a393 into open-mmlab:master Oct 26, 2020
Yshuo-Li pushed a commit to Yshuo-Li/mmediting that referenced this pull request Jul 15, 2022
fix 2 problems in `tool/preprocess_reds_dataset.py`
@yaqi0510
Copy link

Dear wwhio,

First of all, we want to express our gratitude for your significant PR in the MMEditing project. Your contribution is highly appreciated, and we are grateful for your efforts in helping improve this open-source project during your personal time. We believe that many developers will benefit from your PR.

If you are Chinese or have WeChat,welcome to join our community on WeChat. You can add our assistant :openmmlabwx. Please add "mmsig + Github ID" as a remark when adding friends:)

We would also like to invite you to join our Special Interest Group (SIG) private channel on Discord, where you can share your experiences, ideas, and build connections with like-minded peers. To join the SIG channel, simply message moderator— OpenMMLab on Discord or briefly share your open-source contributions in the #introductions channel and we will assist you. We look forward to seeing you there! Join us :https://discord.gg/raweFPmdzG

Thank you again for your contribution❤

Best regards!@wwhio

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

Successfully merging this pull request may close these issues.

4 participants