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] add domain_filter param #13451

Merged
merged 5 commits into from Sep 1, 2020
Merged

[text analytics] add domain_filter param #13451

merged 5 commits into from Sep 1, 2020

Conversation

iscai-msft
Copy link
Contributor

fixes #13409

kristapratico
kristapratico reviewed Aug 31, 2020
View reviewed changes
sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_models.py Outdated
@@ -64,6 +65,10 @@ def get(self, key, default=None):
return self.__dict__[key]
return default

class PiiEntityDomainType(str, Enum):
"""The different domains of PII entities that users can filter by"""
PHI = "PHI"
Copy link
Member

Choose a reason for hiding this comment

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

can we include a comment for PHI about what it stands for?

Copy link
Member

@maririos maririos Aug 31, 2020 •
edited

Choose a reason for hiding this comment

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

According to the Framework Design Guidance:

"Do not use abbreviations or acronyms in identifies. [...] It is more important for names to be readable than it is for them to be brief. I t is equally important not to use abbreviations and acronyms that are not generally understood"

Because of that, we should rename PHI to ProtectedHealthInformation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like PHI fits better in this case though, since we're abbreviating PII

Copy link
Member

Choose a reason for hiding this comment

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

Even between us we've asked multiple times what PHI is. Pii is more common, so I strongly recommend changing it.
For .NET I will use the complete name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, since .NET is using the full name, I'll make the change

kristapratico
kristapratico reviewed Aug 31, 2020
View reviewed changes
sdk/textanalytics/azure-ai-textanalytics/azure/ai/textanalytics/_text_analytics_client.py
:keyword domain_filter: Filters the response entities to ones only included in your specified domain.
I.e., if set to 'PHI', will only return entities in the Personal Healthcare Information domain.
Currently only supports 'PHI'. See https://aka.ms/tanerpii for more information.
:paramtype domain_filter: str or ~azure.ai.textanalytics.PiiEntityDomainType
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the same :versionadded: directive here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they had it on v3.1-preview.1, just never supported it

@iscai-msft iscai-msft merged commit 7f1e4d8 into Azure:master Sep 1, 2020
@iscai-msft iscai-msft deleted the add_filter branch September 1, 2020 14:52
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 1, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
318b112 
…into add_redacted_text

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 1, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
05be0b7 
…into expose_parse_vault_id

* 'master' of https://github.com/Azure/azure-sdk-for-python: (37 commits)
  [text analytics] add versionadded sphinx documentation (Azure#13450)
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
  clean up reference and tests (Azure#13412)
  Consistent returns (Azure#13245)
  [text analytics] return None for offset and length for v3.0 (Azure#13382)
  edit all authentication files and add a test (Azure#13355)
  Add managed_identity_client_id argument to DefaultAzureCredential (Azure#13218)
  [text analytics] add string-index-type support (Azure#13378)
  [text analytics] fix error response if pii entities is called from v3.0 client (Azure#13383)
  Send spec (Azure#13143)
  Anomaly Detector 3.0.0b2 release (Azure#13351)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 2, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
f7625ea 
…into link_om_sample

* 'master' of https://github.com/Azure/azure-sdk-for-python: (23 commits)
  Int32 serialization (Azure#13452)
  add output to opinion mining sample (Azure#13494)
  Add Document w/ Eng Sys Checks (Azure#13492)
  update version (Azure#13495)
  Remove resources post test (Azure#13379)
  bing_id -> bing_entity_search_api_id (Azure#13491)
  [EventGrid] Read me + improve docstrings (Azure#13484)
  Build AuthenticationRecords from ADFS identity tokens (Azure#13341)
  Support Subject Name/Issuer authentication (Azure#13350)
  Add KeyVaultAccessControlClient for data plane RBAC (Azure#13372)
  [text analytics] Add redacted_text (Azure#13449)
  add python sdk sample (Azure#13338)
  [text analytics] add versionadded sphinx documentation (Azure#13450)
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  ...
rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this pull request Sep 4, 2020
@iscai-msft @rakshith91
[text analytics] add domain_filter param (Azure#13451)
fc02f42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maririos maririos maririos approved these changes

@mssfang mssfang mssfang approved these changes

@deyaaeldeen deyaaeldeen Awaiting requested review from deyaaeldeen

@kristapratico kristapratico Awaiting requested review from kristapratico

Assignees
No one assigned
Labels
Cognitive - Text Analytics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[text analytics] add domain param to pii
4 participants
@iscai-msft @maririos @kristapratico @mssfang