Region cropping problem #27

khurramHashmi · 2019-12-12T18:51:18Z

This branch is ready to merge with master branch. Please test first and then merge.

ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py

requirements.txt

bertsky · 2019-12-13T09:29:27Z

ocrd_anybaseocr/ocrd-tool.json

@@ -76,7 +76,7 @@
      "output_file_grp": ["OCR-D-IMG-DEWARP"],
      "parameters": {
        "imgresize":    { "type": "string",                      "default": "resize_and_crop", "description": "run on original size image"},
-        "pix2pixHD":    { "type": "string", "default":"/home/ahmed/project/pix2pixHD", "description": "Path to pix2pixHD library"},
+        "pix2pixHD":    { "type": "string", "default":"/home/jenckel/pix2pixHD/pix2pixHD", "description": "Path to pix2pixHD library"},


Not a good default. Please provide models via setup package_data or subrepo and a makefile rule.

is it really good practice, to link multiple 100 MB models in the setup?
I'd prefer to provide a link to the sources in the README and output a warning/error if they are missing at runtime

is it really good practice, to link multiple 100 MB models in the setup?

No, of course not. But a subrepo for that (which you can link to with relative paths in your README and tool json default etc) is good practise.

I'd prefer to provide a link to the sources in the README and output a warning/error if they are missing at runtime

Yes, that's fine. (This comment was about the absolute pathname which appeared in the original version)

requirements.txt

bertsky · 2019-12-13T09:38:25Z

ocrd_anybaseocr/cli/ocrd_anybaseocr_tiseg.py

@@ -136,10 +143,6 @@ def _process_segment(self,page_image, page, page_xywh, page_id, input_file, n):
                                   file_grp=self.image_grp
            )     
        page.add_AlternativeImage(AlternativeImageType(filename=file_path, comments=page_xywh['features']))


What does this processor do? Provide a derived image of the page with colors/frames for segments? That's not what segmentation in a PAGE-XML annotation workflow looks like. You need to add regions (and remove existing regions).

I have opened #31 to address this

ocrd_anybaseocr/cli/ocrd_anybaseocr_textline.py

bertsky · 2019-12-13T09:46:52Z

ocrd_anybaseocr/cli/ocrd_anybaseocr_textline.py

+                    points = region.Coords.get_points()
+                    points = points.split(" ")
+
+                    x_min = min(int(points[0].split(",")[0]), int(points[1].split(",")[0]), int(points[2].split(",")[0]), int(points[3].split(",")[0]))
+                    x_max = max(int(points[0].split(",")[0]), int(points[1].split(",")[0]), int(points[2].split(",")[0]), int(points[3].split(",")[0]))
+                    y_min = min(int(points[0].split(",")[1]), int(points[1].split(",")[1]), int(points[2].split(",")[1]), int(points[3].split(",")[1]))
+                    y_max = max(int(points[0].split(",")[1]), int(points[1].split(",")[1]), int(points[2].split(",")[1]), int(points[3].split(",")[1]))
+
+                    if x_max>page_image.size[0]:
+                        x_max = page_image.size[0]-1
+                    if y_max>page_image.size[1]:
+                        y_max = page_image.size[1]-1
+
+                    img__ = page_image.crop((x_min,y_min,x_max,y_max))


This is totally inadequate. What if the region already has alternative images? What if it has @orientation (from deskewing)?

The region_xywh you are passing to _process_segment along with img__ does not match. It will give wrong coordinates.

What made you bypass image_from_segment's region_image?

the problem was exactly that it already had an alternative image
we tried to "merge" the following two pipelines:
raw image --> binarization --> deskew --> cropping --> tiseg ---> textline with
raw image --> block segmentation --(region+alternative image)--> textline
per default block segmentation adds an alternative image for each extracted region which means textline will take the alternative image from the raw image over applying the extracted regions to the binarized/deskewed etc. image (both of which textline needs)
We are aware that the pipeline was not supposed to have multiple branches, but with the dependencies of our methods there is no way for a single straight forward pipeline, so we tried to use the API as best as possible.
we ended up with three choices:
either uncouple the pipelines and perform: raw image --> block segmentation --> binarization --> deskew --> textline independently, apply the regions manually (which we tried to do) or not add an alternative image during block segmentation (which may be better?)
What do you think would be the best option? (We tried to explain the problem before, and I hope its understandable now, but communicating the problem turned out to be difficult.)

per default block segmentation adds an alternative image for each extracted region which means textline will take the alternative image from the raw image over applying the extracted regions to the binarized/deskewed etc. image (both of which textline needs)

This is not a valid sentence in English, and I don't understand it.

We are aware that the pipeline was not supposed to have multiple branches, but with the dependencies of our methods there is no way for a single straight forward pipeline, so we tried to use the API as best as possible.

What the API can give you are filters and selector on operations already performed earlier in the workflow. It cannot provide the workflow itself for a processor! (That's up to the user to decide. But of course, you can document what a good setup is for your tools.)

However, the processor can constrain its input images, so it does not pick up the wrong annotations when there is a freedom of choice, and stops execution when put in the wrong (place of a) pipeline.

So I don't see a problem with the above workflow. Just that your textline segmentation needs 2 input file groups then (to merge their regions I guess).

You could even be very restrictive to prevent other workflows:

binarization: filter binarized,deskewed,cropped page images

deskewing: select binarized, filter deskewed,cropped page images

cropping: select binarized,deskewed, filter cropped page images

tiseg: select binarized,deskewed,cropped page images

block segmentation: filter binarized,deskewed,cropped page images

textline: select binarized,deskewed,cropped region images on input file group 1, filter binarized,deskewed,cropped region images on input file group 2

But you don't need to do this (and I would recommend against it, except where an algorithm just cannot handle the input otherwise).

We tried to explain the problem before, and I hope its understandable now, but communicating the problem turned out to be difficult.

You can always turn to the Gitter Lobby for general questions.

The region_xywh you are passing to _process_segment along with img__ does not match. It will give wrong coordinates.

What made you bypass image_from_segment's region_image?

This has been resolved by 13d855e (although the wrong method still remains as commented code).

But the rest of the discussion was about filters/selectors. You took my advice to the extreme in 0ac8567 – hence my comment below

ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py

Co-Authored-By: Robert Sachunsky <[email protected]>

bertsky · 2019-12-16T13:30:16Z

Regarding the CI failures, I believe you should try to update/renew your CircleCI build cache. Also adding some pip install -U pip would be advisable:

You are using pip version 9.0.1, however version 19.3.1 is available.

…/ocrd_anybaseocr into region_cropping_problem reviewed changes

bertsky

Apart from the problem when no border is annotated, I still recommend against making your processors that strict (maximally filtering/selecting image features). This should mostly be up to workflow configuration (for which you could give hints and recommendations in the README and tool json descriptions).

…/ocrd_anybaseocr into region_cropping_problem

wrznr · 2020-01-14T15:23:16Z

Is it save to merge a PR for which the continuous integration tests failed? Is it save for users to update their working installation?

mjenckel · 2020-01-14T16:41:31Z

problem was related to circleci not finding tensorflow 2.0 (as already mentioned by @bertsky)
should be fixed

bertsky · 2020-01-15T00:54:10Z

There are so many unresolved comments above – it's not clear what additional commits address which problems. But I believe quite a few errors remain. @khurramHashmi You make it extremely hard to collaborate.

mjenckel and others added 16 commits November 6, 2019 15:31

Bug Fixes

bd1418e

merged block segmentation and requirements

c0f8293

textregion are now cropped according to any existing border

ec1c048

added pipeline.py for reproducing the error

0486b78

added shapely to requirements

8d699f5

removed keras from requirements, added tensorflow 2.0

c2f0969

updated code to tensorflow 2.0 version

a938165

updated code to tensorflow 2.0

ab211e5

code compatible for Tensorflow 2.0 now

28b9b1c

tensorflow latest version

ba5efb1

Code compatible with opencv-python-headless now

745eb79

Fixed Pipeline and changed file grp according to pull request

0fd8e51

fixed prediction setup

ee8fe4e

Added Reading Order

ce9c0d6

Ordered Region added

e941321

branch ready to merge with master

77fe750

wrznr reviewed Dec 13, 2019

View reviewed changes

ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

bertsky suggested changes Dec 13, 2019

View reviewed changes

mjenckel and others added 5 commits December 13, 2019 19:29

Update ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py

4dadeaa

Co-Authored-By: Robert Sachunsky <[email protected]>

Update ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py

81caefa

Co-Authored-By: Robert Sachunsky <[email protected]>

Update ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py

76b1795

Co-Authored-By: Robert Sachunsky <[email protected]>

Update requirements.txt, tensorflow version

74f965d

Co-Authored-By: Robert Sachunsky <[email protected]>

Update ocrd_anybaseocr/cli/ocrd_anybaseocr_block_segmentation.py

4146c70

Co-Authored-By: Robert Sachunsky <[email protected]>

khurramHashmi added 3 commits December 17, 2019 10:35

changes mentioned in revew

0ac8567

Merge branch 'region_cropping_problem' of https://github.com/mjenckel…

0c32fec

…/ocrd_anybaseocr into region_cropping_problem reviewed changes

available operation level in enum

9f47c52

bertsky suggested changes Dec 17, 2019

View reviewed changes

khurramHashmi added 2 commits December 19, 2019 16:12

regions taken from image_segment function now

13d855e

3 more region classes added.

6b8102c

khurramHashmi and others added 3 commits January 10, 2020 11:19

colSeparator issue reolved

8b17584

Merge branch 'region_cropping_problem' of https://github.com/mjenckel…

1139093

…/ocrd_anybaseocr into region_cropping_problem

Merge branch 'master' into region_cropping_problem

160bf8e

mahmed1995 merged commit 8a39f3a into master Jan 14, 2020

bertsky mentioned this pull request May 18, 2020

ocrd_utils.coordinates_for_segment: clip to parent? OCR-D/core#489

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Region cropping problem #27

Region cropping problem #27

khurramHashmi commented Dec 12, 2019

bertsky Dec 13, 2019

mjenckel Jan 15, 2020

bertsky Jan 16, 2020

bertsky Dec 13, 2019

bertsky Jan 16, 2020

bertsky Dec 13, 2019

bertsky Dec 13, 2019

mjenckel Dec 13, 2019

bertsky Dec 13, 2019

bertsky Jan 15, 2020

bertsky commented Dec 16, 2019

bertsky left a comment

wrznr commented Jan 14, 2020

mjenckel commented Jan 14, 2020 •

edited

Loading

bertsky commented Jan 15, 2020

Region cropping problem #27

Region cropping problem #27

Conversation

khurramHashmi commented Dec 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertsky commented Dec 16, 2019

bertsky left a comment

Choose a reason for hiding this comment

wrznr commented Jan 14, 2020

mjenckel commented Jan 14, 2020 • edited Loading

bertsky commented Jan 15, 2020

mjenckel commented Jan 14, 2020 •

edited

Loading