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

ocrd_utils.coordinates_for_segment: clip to parent? #489

Open
bertsky opened this issue May 18, 2020 · 5 comments · May be fixed by #492
Open

ocrd_utils.coordinates_for_segment: clip to parent? #489

bertsky opened this issue May 18, 2020 · 5 comments · May be fixed by #492

Comments

@bertsky
Copy link
Collaborator

bertsky commented May 18, 2020

We have the utility function coordinates_for_segment for conversion of polygons from some derived image and its coordinate transform to absolute coordinates of the original image.

This is all the function does, but of course the expectation is more: Whereever that polygon was in the derived image, give me coordinates which I can annotate as final result of segmentation! (And unfortunately, that is also what it is mostly used for.)

However, that would entail also doing the inverse of image_from_polygon, i.e. clipping (or rather masking) the polygon to its parent element. This usually means ensuring the child element's polygon stays within the parent element's polygon. Except for the top-level parent (Page when there is no Border), where this means clipping coordinates to a range between zero and image size.

Thus, not having that functionality in core means segmenters will tend to produce inconsistent coordinates (failing the PAGE validator), and sometimes (e.g. when the page was rotated) even negative coordinates (failing XML validators and other PAGE libraries).

Unfortunately, we already made the first argument the segment's polygon instead of its element (as in coordinates_of_segment – an ugly asymmetry which itself originated from PAGE's Border idiosyncrasy). So we cannot silently enrich coordinates_for_segment by looking into the segment's parent_object_ and clipping the polygon respectively.

Or can we? How about making it polymorphic and checking the type of the first argument, as an opt-in for implicit conversion consistent with parent? To me that seems much better than providing a new function and trying to find processors which need it. We could even transform the function to be exclusively for the new usage by starting to deprecate the old usage at a later point and hunting it down.

@kba @wrznr if you give me your thumbs up I will kick this off with a PR.

@bertsky
Copy link
Collaborator Author

bertsky commented May 18, 2020

(For example, this problem has surfaced in ocrd-tesserocr-segment-line which led to a local workaround there as OCR-D/ocrd_tesserocr#104 and OCR-D/ocrd_tesserocr#120, and in ocrd-anybaseocr-block-segmentation as OCR-D/ocrd_anybaseocr/pull/27)

@bertsky
Copy link
Collaborator Author

bertsky commented May 19, 2020

See here for an idea of what could/should really be done in core.

Working with many different libraries (Shapely, OpenCV, Scikit) it's still surprisingly hard to calculate contour polygons in a way that avoids self-intersections. Maybe we should also encapsulate that demand in a utility function.

@kba
Copy link
Member

kba commented May 22, 2020

How about making it polymorphic and checking the type of the first argument, as an opt-in for implicit conversion consistent with parent?

That's a reasonable solution, you have my 👍

@bertsky
Copy link
Collaborator Author

bertsky commented May 22, 2020

This could also be part of an OCR-D adaptor to PAGE-XML pixel-center coordinate convention – if that does turn out to be the binding interpretation. We could make coordinates_of_segment convert to pixel-below-right (as most libraries demand/provide) and coordinates_for_segment convert back.

Let's discuss this on Tuesday!

@bertsky
Copy link
Collaborator Author

bertsky commented May 27, 2020

Or can we? How about making it polymorphic and checking the type of the first argument, as an opt-in for implicit conversion consistent with parent?

No we cannot! The problem is that at the moment most processors scramble the details of the new segment to be created (identifier, coordinates, content) that segment does not exist yet. The only way this can be done is via an extra kwarg for the parent (as shown in #492), which even increases the asymmetry and idiosyncrasy.

But there's another option: Instead of adding some function or argument to explicitly post-process the coordinates, we could also handle all cases implicitly by augmenting the PAGE-XML DOM (via #479's ocrd_page_user_methods._add_method), like so:

  • for coordinate validity (i.e. no self-intersections, maybe convert from pixel-below-right to pixel-center convention), amend the constructor of CoordsType
  • for coordinate consistency (i.e. no child outside of parent via polygon intersection, always set .parent_object_ as if created from parse tree), write a small back-end function and delegate to it from all segment hierarchy constructors and their .add_*, .set_*, .insert_*_at and .replace_*_at functions.

Pros:

  • current processor implementations don't have to be touched (but we can gather new warnings in log data where extra action is still necessary)
  • single point of checks and conversions – easier maintenance
  • errors/warnings appear very close (callstack-wise) to their cause (i.e. not as decontextualised as during to_xml)
  • errors/warnings will be uniform, independent of the processor (e.g. warning: empty intersection between parent and child in TextRegionType.add_TextLine in myProcessor.process_segment etc.)
  • more intuitive / less margin of error for processor programmers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants