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.

Sign up for GitHub

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

Jump to bottom

Add offset and length to SentenceSentiment #14599

Merged
merged 2 commits into from
Aug 31, 2020
Merged

Add offset and length to SentenceSentiment #14599

merged 2 commits into from
Aug 31, 2020

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Aug 28, 2020

  • Added offset and length to SentenceSentiment.
  • Rearrange offset and length order in other models.

fixes: #13418

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics labels Aug 28, 2020
@mssfang mssfang added this to the [2020] September milestone Aug 28, 2020
@mssfang mssfang self-assigned this Aug 28, 2020
@mssfang mssfang requested a review from samvaity August 31, 2020 04:18
@mssfang mssfang marked this pull request as ready for review August 31, 2020 04:18
@mssfang mssfang requested a review from iscai-msft August 31, 2020 16:38
samvaity
samvaity approved these changes Aug 31, 2020
View reviewed changes
...zure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/models/SentenceSentiment.java
*
* @return The {@link SentimentConfidenceScores}.
* @return The offset of sentence.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Consider updating to start position for the sentence in a document.

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel it is too redundant for having it both in the method description and the return value.

...zure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/models/SentenceSentiment.java
Comment on lines +35 to +36
this.offset = 0;
this.length = 0;
Copy link
Member

Choose a reason for hiding this comment

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

These initializations are only on SentenceSentiment and not on LinkedEntityMatch or CategorizedEntity ?

Copy link
Member Author

Choose a reason for hiding this comment

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

we do have in LinkedEntityMatch: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/textanalytics/azure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/models/LinkedEntityMatch.java#L44

and CategorizedEntity: https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/textanalytics/azure-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/models/CategorizedEntity.java#L56

@mssfang mssfang merged commit 2e2087d into Azure:master Aug 31, 2020
@mssfang mssfang deleted the TA-Encoding-SentimentAnalysis branch March 9, 2021 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iscai-msft iscai-msft iscai-msft approved these changes

@samvaity samvaity samvaity approved these changes

@maririos maririos maririos approved these changes

@deyaaeldeen deyaaeldeen Awaiting requested review from deyaaeldeen

Assignees

@mssfang mssfang

Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Text Analytics
Projects
None yet
Milestone
[2020] September
Development

Successfully merging this pull request may close these issues.

[TA] To support encoding to all endpoints that required
4 participants
@mssfang @maririos @samvaity @iscai-msft