Consistency related changes in form recognizer #11467

rakshith91 · 2020-05-15T19:14:25Z

Rename PageRange to FormPageRange
Remove page_number from FormField
Inherit USReceipt from Recognized form.

rakshith91 · 2020-05-15T19:49:43Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_models.py

@@ -180,7 +180,7 @@ def __repr__(self):
        )[:1024]


-class USReceipt(object):  # pylint: disable=too-many-instance-attributes
+class USReceipt(RecognizedForm):  # pylint: disable=too-many-instance-attributes


Alternatively, instead of inheritng from RecognizedForm we can pass in a recognizedForm as a variable and un-flatten this.

self.fields = kwargs.get("fields", None) self.page_range = kwargs.get("page_range", None) self.pages = kwargs.get("pages", None) self.form_type = kwargs.get("form_type", None)

will be replaced by

self.recognized_form = kwargs.get("recognized_form ", None) # type: ~models.RecognizedForm

@johanste thoughts?

I do remember @kristapratico had thoughts about making people access fields, pages etc through recognized_form, I can't fully remember the details though.

I think I'm slightly more in favor of adding recognized_form myself, since fields, form_type etc aren't commonly called on a USReceipt, but my feelings aren't particularly strong.

(editing in because I previously forgot to mention this): .net currently has RecognizedForm as a property on USReceipt

iscai-msft · 2020-05-15T20:01:01Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_models.py

@@ -120,7 +120,7 @@ def __new__(cls, x, y):
        return super(Point, cls).__new__(cls, x, y)


-class PageRange(namedtuple("PageRange", "first_page last_page")):
+class FormPageRange(namedtuple("FormPageRange", "first_page last_page")):
    """The 1-based page range of the document.


very tiny nit (please feel free to ignore): I think it would be better to have the comment as "The 1-based page range of the form" since we have form in the name and form is more specifically what we're returning the page range of.

This change would also include later parts of this docstring, i.e. changing
:ivar int first_page: The first page number of the document
to
:ivar int first_page: The first page number of the form

iscai-msft · 2020-05-15T20:07:19Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_models.py

@@ -180,7 +180,7 @@ def __repr__(self):
        )[:1024]


-class USReceipt(object):  # pylint: disable=too-many-instance-attributes
+class USReceipt(RecognizedForm):  # pylint: disable=too-many-instance-attributes


This is more of a question: If we inherit from another exposed class, do we include the properties we inherited from that other class in our docstring? I did this when I was working on key vault, but just want to check for sure what we should be doing in this case

we must be doing it imo - since they are "valid" properties. As a customer, i would not want to go check the docsstring for a series of models

ok, that makes a lot of sense. thanks!

iscai-msft · 2020-05-15T20:09:57Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/__init__.py

@@ -24,7 +24,7 @@
    CustomFormModelInfo,
    AccountProperties,
    Point,
-    PageRange,
+    FormPageRange,


Please update the changelog

sdk/formrecognizer/azure-ai-formrecognizer/CHANGELOG.md

kristapratico · 2020-05-18T20:30:30Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_models.py

+        self.receipt_locale = kwargs.get("receipt_locale", "en-US")
+
+    def __repr__(self):
+        return "RecognizedForm(form_type={}, fields={}, page_range={}, pages={}, " \


Should be RecognizedReceipt in repr

iscai-msft · 2020-05-18T20:21:53Z

sdk/formrecognizer/azure-ai-formrecognizer/CHANGELOG.md

+- `PageRange` is renamed to `FormPageRange`
+- `FormField` does not have a page_number.
+- `begin_recognize_receipts` APIs now return `RecognizedReceipt` instead of `USReceipt`
+- `USReceiptType` is renamed to `ReceiptType`


I think we should include that we added RecognizedReceipt class, that it inherits from RecognziedForm and USReceipt inherits from it.

RecognizedReceipt is more of a "middle man" and is mainly for documenting purposes - it should not ( and does not) change the behaviour of UsReceipt - so i dont think it should be in changelog

iscai-msft · 2020-05-18T20:24:24Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_models.py

-        self.page_range = kwargs.get("page_range", None)
-        self.pages = kwargs.get("pages", None)
-        self.form_type = kwargs.get("form_type", None)
-        self.receipt_locale = kwargs.get("receipt_locale", "en-US")


You should also delete receipt_type and receipt_locale from the repr below (this would also require the USReceipt repr test)

nope - USReceipt inherits from RecognizedReceipt - so it will still stay logically in the repr

you're right, my mind totally blanked there. thanks!

iscai-msft · 2020-05-18T20:27:15Z

sdk/formrecognizer/azure-ai-formrecognizer/CHANGELOG.md

@@ -10,13 +10,16 @@
 - Removed `get_form_training_client` from `FormRecognizerClient`
 - Added `get_form_recognizer_client` to `FormTrainingClient`
 - A `HttpResponseError` is now raised if a model with `status=="invalid"` is returned from the `begin_train_model()` or `train_model()` methods
+- `PageRange` is renamed to `FormPageRange`
+- `FormField` does not have a page_number.
+- `begin_recognize_receipts` APIs now return `RecognizedReceipt` instead of `USReceipt`


you also need to update the readme here: https://github.com/Azure/azure-sdk-for-python/blob/42684b2051480f546a4fd563411e9785a7d075f3/sdk/formrecognizer/azure-ai-formrecognizer/README.md#formrecognizerclient to say that it returns list of RecognizedReceipt instead of USReceipt

nice catch - thanks

iscai-msft · 2020-05-18T20:30:50Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_response_handlers.py

@@ -29,7 +29,7 @@ def prepare_us_receipt(response):
    for page in document_result:
        if page.fields is None:
            receipt = USReceipt(
-                page_range=PageRange(first_page=page.page_range[0], last_page=page.page_range[1]),
+                page_range=FormPageRange(first_page=page.page_range[0], last_page=page.page_range[1]),


One question I have about this change: shouldn't we also update prepare_us_receipt to return a list of RecognizedReceipt instead of USReceipt (line 31)?

thats a good question - but at this point since we support only us receipt - we will return the USReceipt - the RecognizedReceipt is for documentation purposes so that we keep it more open for future - it's something we will be able to change internally later

this might be too much for the future, but should we add the ability in RecognizedReciept to map to which of its children it should be?
i.e., adding a dict of receipt-locale to child type {"en-us": USReceipt}, so we could pass in this into to RecognizedReceipt and get out a USReceipt in this case.

@kristapratico what are your thoughts on this, I wasn't in the phone call so not sure what you guys decided for now

Yeah I think that makes sense for the future. v2 will GA with only US sales receipts so we'll add that implementation detail in v2.1.

iscai-msft · 2020-05-18T20:31:51Z

sdk/formrecognizer/azure-ai-formrecognizer/tests/test_repr.py

@@ -97,8 +97,8 @@ def form_page(form_table, form_line):

 @pytest.fixture
 def us_receipt_type():
-    model = _models.USReceiptType(type="Itemized", confidence=1.0)
-    model_repr = "USReceiptType(type=Itemized, confidence=1.0)"
+    model = _models.ReceiptType(type="Itemized", confidence=1.0)


Can you add a test that tests the repr of RecognizedReceipt?

iscai-msft · 2020-05-18T20:32:50Z

sdk/formrecognizer/azure-ai-formrecognizer/azure/ai/formrecognizer/_models.py

+        self.receipt_locale = kwargs.get("receipt_locale", "en-US")
+
+    def __repr__(self):
+        return "RecognizedForm(form_type={}, fields={}, page_range={}, pages={}, " \


This should say 'RecognizedReceipt' instead of 'RecognizedForm'

rakshith91 added 2 commits May 15, 2020 00:34

page range + form field page number

966054a

us receipt

0e2a50e

rakshith91 commented May 15, 2020

View reviewed changes

rakshith91 marked this pull request as ready for review May 15, 2020 19:49

rakshith91 requested review from iscai-msft and kristapratico as code owners May 15, 2020 19:49

rakshith91 requested a review from johanste May 15, 2020 19:52

iscai-msft reviewed May 15, 2020

View reviewed changes

rakshith91 added 2 commits May 18, 2020 12:23

RecognizedReceipt

8aa99eb

Merge branch 'master' into form_recog

21e6fac

rakshith91 commented May 18, 2020

View reviewed changes

sdk/formrecognizer/azure-ai-formrecognizer/CHANGELOG.md Show resolved Hide resolved

Rakshith Bhyravabhotla and others added 2 commits May 18, 2020 12:42

Update sdk/formrecognizer/azure-ai-formrecognizer/CHANGELOG.md

b7c0b1e

async

42684b2

kristapratico reviewed May 18, 2020

View reviewed changes

iscai-msft reviewed May 18, 2020

View reviewed changes

comments

8cfc2fe

iscai-msft self-requested a review May 18, 2020 20:53

iscai-msft approved these changes May 18, 2020

View reviewed changes

kristapratico approved these changes May 18, 2020

View reviewed changes

rakshith91 merged commit 83915fa into Azure:master May 18, 2020

Consistency related changes in form recognizer #11467

Consistency related changes in form recognizer #11467

Uh oh!

Conversation

rakshith91 commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rakshith91 May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iscai-msft May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rakshith91 commented May 15, 2020 •

edited

Loading

rakshith91 May 15, 2020 •

edited

Loading

iscai-msft May 15, 2020 •

edited

Loading