Kylel/2023/hardening codebase

[kyleclo]

Big PR. A lot of it is actually just auto linting/formatting. Here are main changes:

1. For Box, I added some assertions to make sure problematic Boxes are caught at creation time:

        if w < 0 or h < 0:
            raise ValueError(f"Width and height must be non-negative, got {w} and {h}")
        if page < 0:
            raise ValueError(f"Page must be non-negative, got {page}")
        if l < 0 or t < 0:
            raise ValueError(f"Left and top must be non-negative, got {l} and {t}")

This has been helpful for debugging issues & the library assumes "nice" Boxes.

2. For Box, I added another assertion for the is_overlap() method:

    def is_overlap(
        self, other: "Box", x: float = 0.0, y: float = 0, center: bool = False
    ) -> bool:
         ...
        if self.page != other.page:
            return False

3. For Span, I added a merge_boxes: bool = True option to small_spans_to_big_span(). This should preserve the default behavior. But gives us the option to not do that and just create a big Span that has no larger Box. I would prefer that Not-Merging is actually the default behavior, but I think that might break too many things, so maybe in the future.

4. For Box and Span, I added utility method cluster_boxes() and cluster_spans(). It's based on overlap.

5. For Box, I also added utility method shrink(). It's not used anywhere now, but it's helpful for debugging.

6. For BoxGroup and SpanGroup, I've added a new flag in the constructor: allow_overlap: Optional[bool] = False. This should preserve default behavior, which is that it disallows Box or Span within itself to have overlaps. For example, this should catch if a single BoxGroup has duplicate Boxes or a single SpanGroup has duplicate Spans. Otherwise, classes behave as they originally did.

7. For Indexers, I added a new BoxGroupIndexer class that behaves similarly to SpanGroupIndexer. It's not used, but it was helpful for debugging.

8. Besides the above library changes, almost everything else I added was a missing test.

[cmwilhelm]

Changes generally make sense to me; I called out the places I believe to be breaking and have suggested alternatives.

Whenever this is good to go I also suggest we merge/rebase in the latest main (to get CI to rerun) and bump the MMDA version in pyproject.toml

[cmwilhelm]

allow_overlap=False as the default is a behavioral change and will almost certainly break stuff in practice. Can we switch the default to True?

[cmwilhelm]

Same story here as BoxGroup -- False as the default is a breaking change in behavior, and there's no telling how much data we already have that violates it.

[cmwilhelm]

Let's leave the method in but add a logging.warning() with a deprecation notice. This may break clients of mmda that use .xywh

[cmwilhelm]

🤯

[cmwilhelm]

Thanks this is much more informative output.

[cmwilhelm]

Did you find any evidence of these negatively positioned or dimensioned boxes when you were running through things? As with the allow_overlap flags in SpanGroup and BoxGroup, I think it won't be possible to audit what existing data already violates these constraints

[regan-huff]

I would love for this PR to include or be accompanied by documentation for the current best expectations for how SpanGroups and BoxGroups will be used.

I also support the more conservative approach Chris suggests of leaving the defaults in a state that will allow existing/old data to be loaded from the annotation store without throwing an error. But perhaps the documentation can explain that the defaults are for backwards compatibility.

I think what we'll try to do to handle the possible impact of these model changes is attempt to update the mmda version for the whole Semantic Reader more or less simultaneously, so we can hopefully have a tight response loop if we do discover some data incompatibilities with the updated mmda models.