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

fix(predictions): TABLE, CELL & KEY_VALUE_SET blocks are not properly processed #660

Merged
merged 17 commits into from
Aug 10, 2020

Conversation

ruiguoamz
Copy link
Contributor

@ruiguoamz ruiguoamz commented Jul 22, 2020

Description of changes:
Bug fix according to: #641

  • Refactored processText(_ textractTextBlocks: [AWSTextractBlock])

  • Fixed the bug where TABLE, CELL blocks are not properly processed

  • Fixed the bug where KEY_VALUE_SET blocks are not properly processed

  • Updated integration tests

  • Update README.md

  • Replaced AWSMobileClient with Amplify.Auth

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ruiguoamz ruiguoamz added bug Something isn't working predictions Issues related to the Predictions category labels Jul 22, 2020
@ruiguoamz ruiguoamz marked this pull request as ready for review July 22, 2020 18:49
@ruiguoamz ruiguoamz requested a review from royjit July 22, 2020 18:51
@ruiguoamz ruiguoamz changed the title fix(predictions): iteration should not be skipped in IdentifyTextResultTransformers.processText() fix(predictions): TABLE, CELL & KEY_VALUE_SET block are not properly processed Jul 29, 2020
@ruiguoamz ruiguoamz requested review from royjit, lawmicha and wooj2 July 29, 2020 22:53
@ruiguoamz ruiguoamz changed the title fix(predictions): TABLE, CELL & KEY_VALUE_SET block are not properly processed fix(predictions): TABLE, CELL & KEY_VALUE_SET blocks are not properly processed Jul 29, 2020
@ruiguoamz ruiguoamz requested a review from kneekey23 July 29, 2020 22:58
@wooj2
Copy link
Contributor

wooj2 commented Aug 3, 2020

  1. The employmentapp.png test case comes from here:
    https://github.com/aws-samples/amazon-textract-code-samples/tree/master/src-csharp/test-files
    a. The file is still a png file, so, it should not have a .jpg extension.
    b. I think we should just leave it as employmentapp.png

  2. testImageTextWithTables.jpg does not seem to be a good test, because it has the same values in each of the tables.

Copy link
Contributor

@wooj2 wooj2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename the file testImageTextAll.jpg to employmentapp.jpg. textAll is ambiguous -- for example, the image does not have a celebrity, and does not address all cases that we currently support.

]
]
)
Amplify.reset()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you understood my comment -I noticed that there is a call to Amplify.reset() in the two functions:
setUp() and tearDown() -- I do not think it should be in both places. I think it makes sense to have it only in tearDown()

Comment on lines 30 to 53
// Set up Amplify predictions configuration
let predictionsConfig = PredictionsCategoryConfiguration(
plugins: [
"awsPredictionsPlugin": [
"defaultRegion": region,
"identify": [
"identifyEntities": [
"maxFaces": 50,
"collectionId": "", //no collectionid
"region": region
]
],
"convert": [
"transcription": [
"language": "en",
"region": region,
"defaultNetworkPolicy": "auto"
]
]
]
]
)
Amplify.reset()

let amplifyConfig = AmplifyConfiguration(predictions: predictionsConfig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we were configuring our integration tests through code (versus a file)... Was there a discussion among the team that we want to adopt this approach? Was there a problem with the approach we had before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Jithin, there is no specific reason why not using one over the other. Since the other categories use file way of configuring, then I would also adopt that way and it's more convenient.

Copy link
Contributor

@wooj2 wooj2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the code with regards to removing the processSelectionElement function. After that, feel free to adopt additional comments, or just push.

Also, you will want to update the Changelog.

@@ -47,7 +47,6 @@ class IdentifyResultTransformers {
points.append(point)
}
return Polygon(points: points)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can revert changes in this file.

Comment on lines 331 to 343
static func processSelectionElement(_ selectionItemFoundOld: Bool,
selectionStatus: AWSTextractSelectionStatus) -> (Bool, Bool) {
var selectionItemFound = selectionItemFoundOld
var isSelected = false
if !selectionItemFound {
selectionItemFound = true
//TODO: Support multiple selection items found in a single cell
isSelected = selectionStatus == .selected
} else {
Amplify.log.error("Multiple selection items found in single cell")
}
return (isSelected, selectionItemFound)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand trying to re-use code here, but in this case, I think it actually adds complexity. Lets remove this function.

Comment on lines 320 to 322
let result = processSelectionElement(selectionItemFound, selectionStatus: selectionStatus)
isSelected = result.0
selectionItemFound = result.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with:

                        if (!selectionItemFound) {
                            selectionItemFound = true
                            //TODO: http://www.github.com/aws-amplify/amplify-ios/issues/<fill in issue number here>
                            // Support multiple selection items found in a KeyValueSet
                            isSelected = selectionStatus == .selected
                        } else {
                            Amplify.log.error("Multiple selection items found in KeyValueSet")
                        }

Comment on lines 210 to 212
let result = processSelectionElement(selectionItemFound, selectionStatus: selectionStatus)
isSelected = result.0
selectionItemFound = result.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with:

                    if (!selectionItemFound) {
                        selectionItemFound = true
                        //TODO: <link>
                        //Support multiple selection items found in a cell
                        isSelected = selectionStatus == .selected
                    } else {
                        Amplify.log.error("Multiple selection items found in a Cell")
                    }

@@ -10,6 +10,7 @@ import AWSRekognition
import Amplify
import AWSTextract

// swiftlint:disable type_body_length
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to avoid this, you can create a file like:

IdentifyTextResultTransformers+Tables.swift (and similarly with key value)

and then put:

extension IdentifyTextResultTransformers {
    static func processTables(...
    static func processTable(_ tableBlock: AWSText...
    static func constructTableCell(_....
}

@ruiguoamz ruiguoamz merged commit 8b63b70 into main Aug 10, 2020
@ruiguoamz ruiguoamz deleted the prediction/transformer branch August 10, 2020 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working predictions Issues related to the Predictions category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants