I'd use a default of -1; Otherwise element 0 and element None sort to the same place.
I don't see an easy way to do that in python so I guess def must_element_index() = element_index if not None else -1.
0 or -1 == -1

I'm thinking...

sidx = -1 if self.element_index is None else self.element_index

Also works. It felt kinda repetitive to me so I thought of making a function.

Neither is great.

from sycamore.utils.element_sorting import ELEMENT_SORT

ELEMENT_SORT = {
   None: bbox_sort_page, # default; alternately fix code to do sort_mode = "bbox" and remove the optional
   "xycut": xycut_sorted_page,
   "bbox": bbox_sort_page
}
...
ELEMENT_SORT[(sort_mode](page) # or add the return value to bbox_sort_page, but the functional version implies that it isn't an inline change

This is more general and keeps the code the same size so I don't have to ask about refactoring to keep the function size down.

Up to you if you want to move the element sorting functions into a single file or leave them in separate files.

I'll take a look. The semantics of sorted() versus sort() are tricky if we want to keep efficiency.

self.element_sort = ELEMENT_SORT[sort_mode]

self.element_sort(document)

Explain in slack to me how we can need to finalize; If the width >= 0 then gen_overlap found a cut point before and after since on the last element it will generate a width of -1, which would fall under the choice is None branch.

Once I understand, add a comment explaining to future readers.

The reason for finalize is so the NodeLeaf will end up with 0 elements if nothing is ever appended.

if False:
   debugstuff

or remove.

bbox sort the remaining elements if you can't split; or document why we leave the elements in the original order.

Do you mean a simple sort by Y if we can't split, or calling the algorithm we're replacing. Aside from the top-level call, the inputs are already sorted for the previous cut. If we can't make a single cut, the current code leaves the order alone. Absent any cuts, is it safe to sort arbitrarily by Y? I could sort by Y as a pre-step.

I think that sorting should define the order, and xycut with bbox fallback is a complete ordering. Otherwise the order is potentially random (and we've had problems in the past with failing to preserve element ordering)

If we can't split at this point, it's because there aren't enough elements. This is the exit from the recursion.

if False: ...

Document why you might not want to update indices. If you don't update indices then round-tripping the document through opensearch will revert the order.
I could imagine a preserve_original_indices, but I have no use case for that so still wouldn't include it.

I'm copying behavior somebody added to bbox_sort. I have no idea.

Ask them and then document in both places. Code where no one knows why its there doesn't make sense.

Unused. Nuking. Easy to revive.

I was assuming that the fix would happen in the code that calls the ML model. I don't expect other users to handle bbox out of order, so I don't see a good reason to handle it here (other than assert).

I think that sorting should define the order, and xycut with bbox fallback is a complete ordering. Otherwise the order is potentially random (and we've had problems in the past with failing to preserve element ordering)

Ask them and then document in both places. Code where no one knows why its there doesn't make sense.