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

Rename "VisualLinker" to "PdfVisualParser" to welcome "HocrVisualParser" #518

Merged
merged 7 commits into from
Oct 3, 2020

Conversation

HiromuHota
Copy link
Contributor

@HiromuHota HiromuHota commented Sep 25, 2020

Description of the problems or issues

Is your pull request related to a problem? Please describe.

As #509 now became a big PR, I'll break it down into smaller PRs.
To support hOCR (#476), this PR renames VisualLinker to PdfVisualParser to welcome HocrVisualParser, which will be submitted in a separate PR.

Does your pull request fix any issue.

N/A.

Description of the proposed changes

  • Renamed VisualLinker to PdfVisualParser, which takes pdf_path as an argument
    • pdf_path now should be a path to a directory containing PDF files.
    • The PDF file's name should be the same as document_name + ".pdf" (or ".PDF")
  • Changed Parser's signature:
    • Renamed vizlink to visual_parser.
    • Removed pdf_path. Now this is required only by PdfVisualParser.
    • Removed visual. Use visual_parser if visual information is to be parsed.

Test plan

A clear and concise description of how you test the new changes.

Checklist

  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the CHANGELOG.rst accordingly.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2020

Codecov Report

Merging #518 into master will increase coverage by 0.08%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #518      +/-   ##
==========================================
+ Coverage   85.72%   85.81%   +0.08%     
==========================================
  Files          88       90       +2     
  Lines        4583     4582       -1     
  Branches      856      852       -4     
==========================================
+ Hits         3929     3932       +3     
+ Misses        468      467       -1     
+ Partials      186      183       -3     
Flag Coverage Δ
#unittests 85.81% <94.44%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
src/fonduer/parser/visual_parser/visual_parser.py 80.00% <80.00%> (ø)
src/fonduer/parser/parser.py 93.15% <100.00%> (+1.07%) ⬆️
src/fonduer/parser/visual_parser/__init__.py 100.00% <100.00%> (ø)
.../fonduer/parser/visual_parser/pdf_visual_parser.py 84.76% <100.00%> (ø)

@HiromuHota HiromuHota marked this pull request as ready for review September 30, 2020 00:09
@HiromuHota HiromuHota mentioned this pull request Sep 30, 2020
7 tasks
@HiromuHota HiromuHota requested a review from senwu September 30, 2020 18:07
@HiromuHota HiromuHota added the enhancement New feature or request label Sep 30, 2020
Copy link
Collaborator

@senwu senwu left a comment

Choose a reason for hiding this comment

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

Left two comments. Otherwise looks good to me.

tests/parser/test_parser.py Outdated Show resolved Hide resolved
tests/parser/test_parser.py Show resolved Hide resolved
@HiromuHota
Copy link
Contributor Author

@senwu thanks for your review and comments. Please review again and merge it when it's ready.

@senwu senwu merged commit 9b0719d into HazyResearch:master Oct 3, 2020
@HiromuHota HiromuHota deleted the fix/476_1 branch October 3, 2020 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants