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

Extension framework + Feature-based alignment #19

Merged
merged 10 commits into from Mar 15, 2020

Conversation

leongwaikay
Copy link
Contributor

@leongwaikay leongwaikay commented Dec 27, 2019

Quite a number of things in this PR, as I was refactoring and tidying up as I go (Just following the Boy Scouts rule of leaving your code better than you found it). Ideally each new point should be a separate PR on its own but it was too intertwined to separate when I was done with writing.

Summary of changes

  • Added feature-based alignment by using a reference image. Works wonders when there are no page boundaries or markers to realigned a scanned document.
    • See sample5
  • Modularised the image preprocessing into plugins/extensions/modules. Not sure which name fits so I just called them extensions for now. Basically the template can specify what kind of preprocessing is needed to prepare the images for OMR.
    • New preprocessors can just be placed arbitrary nested in the extension directory and as long as they subclass ImagePreprocessor they will be loaded.
    • Added some builtin preprocessors like GaussianBlur and Levels.
    • Move markers into its own module. (Need much testing here as I do not have sheets with markers)
    • Not sure if template is the best place to specify preprocessors as it is dependent on the way the particular images are obtained. Not so much the structure of the form. Maybe we should differentiate between template and configuration file.
  • Keys of template.json are now case insensitive and ordered. Basically, they are converted to lowercase and then placed in an OrderedDict. This is because order matters to the application of the image preprocessors. And it is confusing to keep wondering what case the keys are.
  • Refactored to use pathlib for most filepaths. os.path.join is too unclean. Next step should be to clean up the output file paths.

@leongwaikay leongwaikay changed the title Extension framework + Extension framework + Feature-based alignment Dec 27, 2019
@Udayraj123
Copy link
Owner

Udayraj123 commented Dec 27, 2019

Ideally each new point should be a separate PR on its own but it was too intertwined to separate when I was done with writing.

No problem! As long as the code is improving we won't face such issues in the future.

Need much testing here as I do not have sheets with markers

Yeah, they were updated into sample1 folder. I will test by updating their template.json files (might need a bit help from you here)

Not sure which name fits so I just called them extensions for now

modules would be a better term I think.

Not sure if template is the best place to specify preprocessors

Yes, template.json shall not contain the whole preprocessing config. At-least there should not be a compulsion to supply it in every template(As it is not the user's worry). Also, the --noMarker flag, etc would have an alternative through this, might increase confusion on using both.

Maybe we should differentiate between template and configuration file.

Valid point, maybe an additional config.json file which if present, will override the default config including preprocessing and the flags.

And it is confusing to keep wondering what case the keys are.

We can follow camelCasing for all keys, lowercase would cause readability issues.

Next step should be to clean up the output file paths.

Would look into it :)

extensions/Markers.py Show resolved Hide resolved
extensions/CropPage.py Show resolved Hide resolved
main.py Show resolved Hide resolved
@Udayraj123
Copy link
Owner

This branch would contain my changes(conflict resolutions and sample case fixes etc) over your commits for merging this PR with conflicts-
https://github.com/Udayraj123/OMRChecker/tree/leongwaikay-extension-framework

@Udayraj123
Copy link
Owner

Udayraj123 commented Feb 25, 2020

@leongwaikay just giving update - I will soon find some time to finish reviewing this PR(with a fabulous contribution of +1000-400). Thanks for being patient!

@Udayraj123
Copy link
Owner

Udayraj123 commented Mar 11, 2020

@leongwaikay I am currently testing out my samples with the new configurations. Specifically, using the Markers.py extension(now renamed to CropOnMarkers.py) with sample1.
I see that getBestMatch(...) from utils.py is not yet ready to accept the new ImageProcessor.

Exception has occurred: AttributeError
module 'config' has no attribute 'marker_rescale_range'
  File "/OMRChecker/utils.py", line 267, in getBestMatch
    config.marker_rescale_range[1] - config.marker_rescale_range[0]) // config.marker_rescale_steps
  File "/OMRChecker/extensions/CropOnMarkers.py", line 78, in apply_filter
    best_scale, allMaxT = utils.getBestMatch(image_eroded_sub, self.marker)
  File "/OMRChecker/main.py", line 332, in process_files
    inOMR = pp.apply_filter(inOMR, args)
  File "/OMRChecker/main.py", line 85, in process_dir
    process_files(omr_files, template, args_local, output_set)
  File "/OMRChecker/main.py", line 93, in process_dir
    process_dir(root_dir, d, template)
  File "/OMRChecker/main.py", line 93, in process_dir
    process_dir(root_dir, d, template)
  File "/OMRChecker/main.py", line 582, in <module>
    process_dir(Path(root), Path(root), args['template'])

Will fix this so that Older samples also get compatible with this configuration

Edit: Fix added(with many other changes for making sample1 run) :-4cdc4fe

@Udayraj123
Copy link
Owner

Udayraj123 commented Mar 11, 2020

@leongwaikay I have made changes in your branch to make it compatible with my samples.
Edit: Merged now. Cheers!

@udayrajMT udayrajMT merged commit 214d02b into Udayraj123:dev Mar 15, 2020
@Udayraj123 Udayraj123 mentioned this pull request Sep 30, 2022
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.

None yet

3 participants