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.

Sign up for GitHub

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

Jump to bottom

Updating the readme #938

Merged
merged 6 commits into from
Jul 4, 2022
Merged

Updating the readme #938

merged 6 commits into from
Jul 4, 2022

Conversation

Atomme1
Copy link
Contributor

@Atomme1 Atomme1 commented Jun 1, 2022

I'm proposing to change the JSON file section to match the real JSON file format you have to implement when doing an recognition training.
Because i have encountered some problem with it ^^'
Cheers 🥂

@Atomme1
Updating the readme
44cd460 
I'm proposing to change the JSON file section to match the real JSON file format you have to implement when doing an recognition training.
Because i have encountered some problem with it ^^' 
Cheers 🥂
frgfm
frgfm previously approved these changes Jun 1, 2022
View reviewed changes
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Atomme1 🙏
I added a small suggestion, let me know what you think!

references/recognition/README.md Show resolved Hide resolved
frgfm
frgfm requested changes Jun 1, 2022
View reviewed changes
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Small mistake of positioning the # labels.json :)

references/recognition/README.md Show resolved Hide resolved
frgfm
frgfm requested changes Jun 1, 2022
View reviewed changes
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

My bad, I wasn't very clear 😅

references/recognition/README.md Outdated
@@ -54,6 +53,7 @@ The order of entries in the json does not matter.
...
}
```
# labels.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant right after the line of the shell 🙃 like at the top within the code formatted section, above the opening bracket

@codecov
Copy link

codecov bot commented Jun 1, 2022 •
edited
Loading

Codecov Report

Merging #938 (857b632) into main (9530f81) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #938   +/-   ##
=======================================
  Coverage   95.16%   95.16%           
=======================================
  Files         134      134           
  Lines        5520     5520           
=======================================
  Hits         5253     5253           
  Misses        267      267
Flag Coverage Δ
unittests 95.16% <ø> (ø)

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

Impacted Files Coverage Δ
doctr/transforms/modules/base.py 94.59% <0.00%> (ø)
doctr/transforms/functional/base.py 95.65% <0.00%> (ø)

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 9530f81...857b632. Read the comment docs.

frgfm
frgfm reviewed Jun 1, 2022
View reviewed changes
references/recognition/README.md
...
}
```

When typing your labels, be aware that the VOCAB doesn't handle spaces.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick note here, we should specify that it's because text recognition is expecting word-level crops (hence no spaces)

What do you think @charlesmindee @felixdittrich92 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Atomme1 @frgfm I agree, that would be good 👍 Maybe update the text detection README (in this PR) with a short note also ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this in another PR 👍 unless @Atomme1 is willing to update this one

@felixdittrich92 felixdittrich92 added topic: documentation Improvements or additions to documentation ext: references Related to references folder awaiting response Waiting for feedback labels Jun 13, 2022
frgfm
frgfm approved these changes Jun 23, 2022
View reviewed changes
Copy link
Collaborator

@frgfm frgfm left a comment

Choose a reason for hiding this comment

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

Thanks @Atomme1 🙏

Let's open another PR for the mention of word-level crops

@frgfm frgfm added the type: enhancement Improvement label Jun 23, 2022
@frgfm frgfm self-assigned this Jun 23, 2022
@frgfm frgfm added this to the 0.6.0 milestone Jun 23, 2022
@frgfm
Copy link
Collaborator

frgfm commented Jun 23, 2022

Would you mind merging the "main" branch into your branch @Atomme1 please? 🙏

@Atomme1
Copy link
Contributor Author

Atomme1 commented Jun 27, 2022

Would you mind merging the "main" branch into your branch @Atomme1 please? 🙏

Heyy, sorry for the late response, i just merge the main into mybranch. But i don't have access to merge my branch on yours

@frgfm
Copy link
Collaborator

frgfm commented Jul 1, 2022

Would you mind merging the "main" branch into your branch @Atomme1 please? pray

Heyy, sorry for the late response, i just merge the main into mybranch. But i don't have access to merge my branch on yours

Sorry @Atomme1, I think you need to merge the main branch again to fix the CI :)

@Atomme1
Copy link
Contributor Author

Atomme1 commented Jul 4, 2022

Would you mind merging the "main" branch into your branch @Atomme1 please? pray

Heyy, sorry for the late response, i just merge the main into mybranch. But i don't have access to merge my branch on yours

Sorry @Atomme1, I think you need to merge the main branch again to fix the CI :)

Done ^^

@frgfm
Copy link
Collaborator

frgfm commented Jul 4, 2022

@Atomme1 I think either you forgot to push the merge, or forgot to update the upstream first :)
I just checked your branch and this line https://github.com/Atomme1/doctr/blob/patch-2/tests/conftest.py#L67
should look like this https://github.com/mindee/doctr/blob/main/tests/conftest.py#L82

@frgfm
Copy link
Collaborator

frgfm commented Jul 4, 2022

To do so:

git checkout main
git fetch upstream
git merge upstream/main
git push origin main
git checkout patch-2
git merge main

then you can push :)

@Atomme1
Copy link
Contributor Author

Atomme1 commented Jul 4, 2022

Okay i think i did it ?
(i needed to reopen the project on my pycharm xD )

@felixdittrich92 felixdittrich92 removed the awaiting response Waiting for feedback label Jul 4, 2022
@felixdittrich92 felixdittrich92 merged commit 2e9a4e1 into mindee:main Jul 4, 2022
@felixdittrich92
Copy link
Contributor

@Atomme1 Thanks a lot for applying all these updates 🤗

@Atomme1
Copy link
Contributor Author

Atomme1 commented Jul 4, 2022

@Atomme1 Thanks a lot for applying all these updates 🤗

Lessgooo it worked !
A pleasure to help the team 😄

@felixdittrich92 felixdittrich92 mentioned this pull request Sep 26, 2022
Closed
85 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixdittrich92 felixdittrich92 felixdittrich92 left review comments

@frgfm frgfm frgfm approved these changes

Assignees

@frgfm frgfm

Labels
ext: references Related to references folder topic: documentation Improvements or additions to documentation type: enhancement Improvement
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Atomme1 @frgfm @felixdittrich92