Processor result object

[kba]

Just a quick draft, to be refined tomorrow.

[kba]

OCR-D/ocrd_kraken#44 is adapted.

[bertsky]

Thanks for starting this and spotting all these typing errors!

I would prefer calling the new class OcrdPageResult instead of OcrdProcessResult, because

• this is about PcGts / OcrdPage objects primarily
• this is about the single-page function, while "processing" and "processor" in general refer to workspace operations
• we have no OcrdProcess

Also, I wonder if it is really necessary to raise this to ocrd_models – it's meant to be an internal interface between ocrd.Processor.process_page_pcgts and ocrd.Processor.process_page_file.

Instead of making images a list of tuples again, why not define a data class with members like pil / file_id / file_path?

More importantly, let's go one step further and

• replace the file ID with just the file ID suffix to be added to the PcGts file ID (that way, process_page_pcgts does not need to know about the output file ID at all)
• replace the image path with the reference to the generated/annotated AlternativeImageType in the resulting PcGtsType, so the calling process_page_file can simply set its pathname after writing the image file (that way, process_page_pcgts does not need to know the output file path in advance)

BTW, I think you forgot to add ocrd_models.ocrd_process_result.py.

[kba]

BTW, I think you forgot to add ocrd_models.ocrd_process_result.py.

Well, crap, I just git checkouted that away after switching branches. I'll rewrite it.

I would prefer calling the new class OcrdPageResult instead of OcrdProcessResult, because

Agree. Done

Instead of making images a list of tuples again, why not define a data class with members like pil / file_id / file_path?

No reason really, I'll add OcrdPageResultImage Done

• replace the file ID with just the file ID suffix to be added to the PcGts file ID (that way, process_page_pcgts does not need to know about the output file ID at all)

Will do. Done

• replace the image path with the reference to the generated/annotated AlternativeImageType in the resulting PcGtsType, so the calling process_page_file can simply set its pathname after writing the image file (that way, process_page_pcgts does not need to know the output file path in advance)

Will also do. Done

[kba]

Also, I wonder if it is really necessary to raise this to ocrd_models – it's meant to be an internal interface between ocrd.Processor.process_page_pcgts and ocrd.Processor.process_page_file.

No reason now, I just like that all the "dumb" data classes are in one spot. But I'll move it/reimplement it closer to processor.base.

[kba]

BTW, I think you forgot to add ocrd_models.ocrd_process_result.py.

Well, crap, I just git checkouted that away after switching branches. I'll rewrite it.

I would prefer calling the new class OcrdPageResult instead of OcrdProcessResult, because

Agree

Instead of making images a list of tuples again, why not define a data class with members like pil / file_id / file_path?

Done and OCR-D/ocrd_kraken#44 adapted accordingly. Now to change the interface.

[kba]

OK, I think I have everything together now, interface-wise. Now adapting kraken, looking forward to simplify binarize in particular ;)

[kba]

I think we can go even further with simplifying the handling of alternative images, but I'll do that after the a1.

[kba]

OK, I think I have everything together now, interface-wise. Now adapting kraken, looking forward to simplify binarize in particular ;)

Done

[bertsky]

Excellent!

[bertsky]

Why Optional (also in function prototype)?

[kba]

Here: Because we're instantiating a list of None values, which are not OcrdPage.

In the function signature of process_page_pcgts: Same situation, there might be "holes" in the list of input_pcgts when any of the input_files in process_page_files cannot be parsed as PAGE-XML.

And for process_page_files: The input_files can be hole-y, if the workspace.download_file fails for any of the files (beyond the first?).

But really, I was trying to make sure that static type checking had no more complaints. I tried to add assert statements where I know that variables must be defined or of a certain type to mitigate the "everything might be None" problem somewhat.

[bertsky]

Oh, right, I forgot about the holes returned by zip_input_files for multiple fileGrps but incomplete PAGE-XML coverage per page!

Maybe we should document this more loudly.

[bertsky]

Suggested change

	except ValueError as e:
	except (AssertionError, ValueError) as e:

[kba]

Can this ever happen, ie. can page_from_file(with_etree=False) ever return anything other than a PcGtsType? I think if that was ever the case, we'd want that AssertionError to be raised because then we'd have broken something.

[bertsky]

You're right – it cannot happen. But then what is the assertion good for – satisfying the type checker?

[kba]

First, my curiosity that I understand the behavior correctly. But secondly, yes, the type checker ;)

[kba]

But reading this again, I should have used OcrdPage not PcGtsType, which is just an alias but we use OcrdPage in the method typing.

[bertsky]

Yes. Feel free to change in OCR-D#1240.

[kba]

Merged into OCR-D#1240 for the a1 release, we can still discuss the Optional weirdness and AssertionError here or there of course.

[bertsky]

we can still discuss the Optional weirdness and AssertionError here or there of course.

no need to – thanks!