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.
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
CodeCamp #116 Add SROIE to dataset preparer #1639
CodeCamp #116 Add SROIE to dataset preparer #1639
Changes from 6 commits
a6f3e44
eddfbf9
b2b8479
e995a30
be1731d
aa77250
1f7920a
163ac9e
744374b
5a300b5
bda6569
62c7e14
2fff4ed
d7d1381
e43c136
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines can be optimized to be more general. The
if
condition is too strict and may only be applicable to this dataset.glob
to match all the files whenever there is*
in the file path.mmengine
has a utilmkdir_or_exist
to create the directory whenever needed.glob.glob
should return a list of existing files.if
block doesn't make the logic flow very clear. I'm in favor ofif...elif...
hereTo conclude, the code can be rewritten as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestions and I have modified this code block. But with #1639 (comment) fixed, this reports error like 'path already exists' when running task-recog after task-det. Keeping the
not osp.exists(dst)
inif
statement will be fine. So it's like thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to address the key issue. Such an error occurs because
shutil
will check the existence of the destination file. Ifdst
is just a directory path,shutil
will do the check on the resulting filename, which is equivalent toosp.exists(osp.join(dst, osp.basename(src))
.Back to your implementation:
... and not osp.exists(dst)
only verifies the existence of the directory instead of a specific file. Suppose that we have placed an empty directory todst
, this implementation will still skip moving all the files inside.Therefore, it's better to just check the existence of each file with
osp.exists(osp.join(dst, osp.basename(f))
and only skip those existing ones.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference with ICDARTextDetAnnParaser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference is the try-except structure added to handle unicode errors that I want to skip. Should I merge this into the ICDARTextDetAnnParser?