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

Migrate Python code to a dedicated package #309

Merged
merged 8 commits into from
Jan 8, 2023
Merged

Migrate Python code to a dedicated package #309

merged 8 commits into from
Jan 8, 2023

Conversation

stefan6419846
Copy link
Contributor

This is my attempt to migrate the existing code for working with artificial training data to a dedicated Python package, as proposed in #308 and #307. This includes some additional refactoring to the module structure to better encapsulate specific functionality.

I have used version number 0.1 for now, although I am up to changing this.

When migrating, I had two parameters which I am not sure about:

  • overwrite, defaulting to False, does not seem to be used at all.

  • It has not been clear enough for me what extract_font_properties really means and therefore misses documentation. text2image --help did not really help me in this case as well:

    --only_extract_font_properties Assumes that the input file contains a list of ngrams. Renders each ngram, extracts spacing properties and records them in output_base/[font_name].fontinfo file. (type:bool default:false)

    What would be an appropriate documentation of the parameter?

If there is anything unclear or you want to see anything changed about this, feel free to ask or report.

@Shreeshrii
Copy link
Collaborator

Thanks. A dedicated package for training from fonts is a good idea.

You may want to look at the original bash scripts that were sought to be replicated in these python scripts in older versions eg. https://github.com/tesseract-ocr/tesseract/tree/4.0/src/training

'overwrite' if I recall correctly was used for the legacy training offered in the bash scripts.

@stefan6419846
Copy link
Contributor Author

src/README.md Outdated Show resolved Hide resolved
src/setup.py Outdated
'Topic :: Scientific/Engineering :: Image Recognition',
'License :: OSI Approved :: Apache Software License',
'Programming Language :: Python :: 3',
'Programming Language :: Python :: 3.6',
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
'Programming Language :: Python :: 3.6',

src/setup.py Outdated
],
keywords='Tesseract,tesseract-ocr,OCR,optical character recognition',

python_requires='>=3.6',
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
python_requires='>=3.6',
python_requires='>=3.7',

@stweil
Copy link
Collaborator

stweil commented May 24, 2022

Font properties (bold, italic, ...) are also from the legacy training and still unsupported with the LSTM recognizer. That's one of the reasons why there remains a certain need for legacy models. The old tesstrain.sh supported training of legacy models, and I think that it would be good to support it in the Python code, too. That should be done separately, not in this pull request here, but maybe you can keep the corresponding parameters with appropriate TODO comments.

@stefan6419846
Copy link
Contributor Author

@stweil Do you mean that we should keep the existing parameters for now when you are talking about the legacy support? Or does this refer to the linedata parameter only?

src/README.md Outdated Show resolved Hide resolved
@stweil
Copy link
Collaborator

stweil commented May 24, 2022

@stweil Do you mean that we should keep the existing parameters for now when you are talking about the legacy support? Or does this refer to the linedata parameter only?

I'd keep all existing parameters for now (with comments).

@stefan6419846
Copy link
Contributor Author

I have updated the requirements inside the README and fixed the parameters for tesstrain.wrapper.run().

@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issues which require input by the reporter which is not provided label Jul 10, 2022
@stefan6419846
Copy link
Contributor Author

Is there anything to be changed here to get this PR merged?

@stweil stweil removed the stale Issues which require input by the reporter which is not provided label Jul 11, 2022
.gitignore Outdated Show resolved Hide resolved
@@ -0,0 +1,160 @@
# Byte-compiled / optimized / DLL files
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole file looks like a copy from somewhere else. Which parts are really required? Why can those parts not be included in the root .gitignore?

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 this is the standard GitHub Python .gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the default Python .gitignore template which usually is recommended for Python projects.

Only keeping the relevant entries might be an option, mainly concerning specific libraries which are currently unused. Nevertheless, I usually prefer to keep the default .gitignore file as it usually is some type of common sense.

I did not add this to the root .gitignore file to avoid unintended side effects for now, as the whole repository consists of two more or less independent parts, while not being clearly separated for now (see #307).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stweil I have reduced the .gitignore to the entries which actually make sense in this use case (at least from my point of view).

Do you still want me to migrate the corresponding entries to the root .gitignore file?

@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stefan6419846
Copy link
Contributor Author

Any further update on this?

@stale
Copy link

stale bot commented Nov 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issues which require input by the reporter which is not provided label Nov 2, 2022
@stweil stweil removed the stale Issues which require input by the reporter which is not provided label Nov 2, 2022
@zdenop
Copy link
Contributor

zdenop commented Jan 5, 2023

Can this be merged?

@stefan6419846
Copy link
Contributor Author

At least from my side, yes. @stweil had some objections about the .gitignore file, but I did not yet hear back after my latest request for further changes which might be required.

@stweil
Copy link
Collaborator

stweil commented Jan 8, 2023

I'd remove any reference to Python 3.6 (see my two comments). .gitignore still contains lots of entries which are not strictly necessary, but that is not critical for merging.

@stefan6419846
Copy link
Contributor Author

I have removed the references to Python 3.6 and updated the README to make clear that Tesseract version 5 is supported as well.

Copy link
Collaborator

@stweil stweil left a comment

Choose a reason for hiding this comment

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

Let's try the new code. Thank you @stefan6419846 for your patience.

@stweil stweil merged commit f4103bd into tesseract-ocr:main Jan 8, 2023
@zdenop
Copy link
Contributor

zdenop commented Jan 8, 2023

@stweil: what about tagging the previous code/commit as version 1.0?
So we can maybe do more reorganization of code without breaking somebody's workflow.
I am interested to make training on windows just by using python. If python is required then Auxiliaries are really not needed.

@stefan6419846 stefan6419846 deleted the pip_package branch January 8, 2023 19:40
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.

5 participants