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

[Text Analytics] Better names for assertions #14070

Merged
merged 1 commit into from
Mar 3, 2021
Merged

[Text Analytics] Better names for assertions #14070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions sdk/textanalytics/ai-text-analytics/review/ai-text-analytics.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ export interface AnalyzeSentimentSuccessResult extends TextAnalyticsSuccessResul
export interface AssessmentSentiment extends SentenceAssessment {
}

// @public
export type Association = "subject" | "other";

export { AzureKeyCredential }

// @public
Expand All @@ -114,12 +111,6 @@ export interface BeginAnalyzeHealthcareEntitiesOptions extends TextAnalyticsOper
export interface CategorizedEntity extends Entity {
}

// @public
export type Certainty = "Positive" | "Positive Possible" | "Neutral Possible" | "Negative Possible" | "Negative";

// @public
export type Conditionality = "Hypothetical" | "Conditional";

// @public
export interface DetectedLanguage {
confidenceScore: number;
Expand Down Expand Up @@ -217,20 +208,29 @@ export interface ExtractKeyPhrasesSuccessResult extends TextAnalyticsSuccessResu
keyPhrases: string[];
}

// @public (undocumented)
export interface HealthcareAssertion {
association?: Association;
certainty?: Certainty;
conditionality?: Conditionality;
}

// @public
export interface HealthcareEntity extends Entity {
assertion?: HealthcareAssertion;
assertion?: HealthcareEntityAssertion;
dataSources: EntityDataSource[];
normalizedText?: string;
}

// @public (undocumented)
export interface HealthcareEntityAssertion {
association?: HealthcareEntityAssociation;
Copy link
Member

Choose a reason for hiding this comment

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

EntityAssociation should be good enough, since it is under the HealthcareEntityAssertion. That logic also applies to EntityCertainty and EntityConditionality

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 already use the Healthcare prefix for many types, e.g.:
HealthcareEntityRelation,
HealthcareEntityRelationRole,
HealthcareEntityRelationRoleType

So I think we should be consistent and use the prefix Healthcare.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @maririos @suhas92 @iscai-msft @abhahn

Copy link
Member

Choose a reason for hiding this comment

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

I don't necessarily agree. The relations, roles, and types, are in fact specific to Healthcare.
The association, certainty, and conditionally could apply to a general entity. I understand it is not the case now, but it will be good to ask the service team if this concept could ever be applied to other entities/endpoints

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I asked Ashly and he said there could be plans to add assertions for other type of entities in one or two semesters. Based on this, I will remove the Healthcare prefix.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the follow up Deya!

certainty?: HealthcareEntityCertainty;
conditionality?: HealthcareEntityConditionality;
}

// @public
export type HealthcareEntityAssociation = "subject" | "other";
Copy link
Member

Choose a reason for hiding this comment

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

Does JS/TS need to capitalize the first letter, such as "Subject", "Other"?

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 am just following the swagger, does this needs to be fixed in the swagger?


// @public
export type HealthcareEntityCertainty = "Positive" | "PositivePossible" | "NeutralPossible" | "NegativePossible" | "Negative";

// @public
export type HealthcareEntityConditionality = "Hypothetical" | "Conditional";

// @public
export interface HealthcareEntityRelation {
relationType: HealthcareEntityRelationType;
Expand Down
6 changes: 3 additions & 3 deletions sdk/textanalytics/ai-text-analytics/src/generated/models/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1102,9 +1102,9 @@ export type Conditionality = "Hypothetical" | "Conditional";
/** Defines values for Certainty. */
export type Certainty =
| "Positive"
| "Positive Possible"
| "Neutral Possible"
| "Negative Possible"
| "PositivePossible"
| "NeutralPossible"
| "NegativePossible"
| "Negative";
/** Defines values for Association. */
export type Association = "subject" | "other";
Expand Down
8 changes: 4 additions & 4 deletions sdk/textanalytics/ai-text-analytics/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,10 @@ export {
TokenSentimentValue,
TextAnalyticsWarning,
State as TextAnalyticsOperationStatus,
HealthcareAssertion,
HealthcareAssertion as HealthcareEntityAssertion,
PiiCategory as PiiEntityCategory,
Association,
Certainty,
Conditionality,
Association as HealthcareEntityAssociation,
Certainty as HealthcareEntityCertainty,
Conditionality as HealthcareEntityConditionality,
RelationType as HealthcareEntityRelationType
} from "./generated/models";