Skip to content

Review main-notebooks/build_person_directory.ipynb #53

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Review main-notebooks/build_person_directory.ipynb #53

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
118 changes: 61 additions & 57 deletions notebooks/build_person_directory.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"source": [
"## Objective\n",
"\n",
" This notebook demonstrates how to identify faces in an image against a known set of persons. It begins by building a Person Directory, where each subfolder in a specified directory represents an individual. For each subfolder, a person is created and all face images within it are enrolled to that person.\n",
"This notebook demonstrates how to identify faces in an image by matching them against a known set of persons. It begins by building a Person Directory, where each subfolder in a specified directory represents an individual. For each subfolder, a person entry is created, and all face images within that folder are enrolled to that person.\n",
"\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Grammar, Clarity]
    • change: Removed leading space and rephrased the opening sentence for smoother reading ("against a known set of persons" to "by matching them against a known set of persons"). Changed "person" to "person entry" and "subfolder" to "folder" for consistency.
    • rationale: Improved sentence structure and clarity by making the explanation more precise and readable. Replacing "person" with "person entry" clearly indicates that an entry is being created, not just referencing a person. Changing "subfolder" to "folder" aligns wording for better readability.
    • impact: Enhances the clarity and professionalism of the documentation, making it easier for readers to understand the process described.

"| Enrollment | Searching |\n",
"| :-: | :-: |\n",
Expand All @@ -27,15 +27,19 @@
"id": "80a375ca",
"metadata": {},
"source": [
"## Create Azure content understanding face client\n",
"> The [AzureContentUnderstandingFaceClient](../python/content_understanding_face_client.py) is a utility class for interacting with the Content Understanding Face service. Before the official SDK is released, this acts as a lightweight SDK. Set the constants **AZURE_AI_ENDPOINT**, **AZURE_AI_API_VERSION**, and **AZURE_AI_API_KEY** with your Azure AI Service information.\n",
"## Create Azure Content Understanding Face Client\n",
"\n",
"> The [AzureContentUnderstandingFaceClient](../python/content_understanding_face_client.py) is a utility class designed for interacting with the Content Understanding Face service. Before the official SDK is released, this acts as a lightweight SDK.\n",
"\n",
"> Set the constants **AZURE_AI_ENDPOINT**, **AZURE_AI_API_VERSION**, and **AZURE_AI_API_KEY** with your Azure AI Service credentials.\n",
"\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Formatting, Clarity]

    • change: Added a blank line after the heading "## Create Azure Content Understanding Face Client"
    • rationale: Creating a visual separation improves readability and follows markdown best practices
    • impact: Enhances document structure, making it easier for readers to distinguish sections

  • categories: [Grammar, Clarity]

    • change: Revised description from "is a utility class for interacting with ..." to "is a utility class designed for interacting with ..."
    • rationale: Improves sentence flow and clarity by using "designed for" instead of the more casual "for"
    • impact: Provides a more polished and professional tone, making the description clearer to readers

  • categories: [Clarity]

    • change: Split one long sentence into two separate sentences, moving the statement about setting constants to a new line
    • rationale: Separating distinct ideas improves readability and comprehension
    • impact: Helps users better understand the instructions for setting credentials without confusion

  • categories: [Clarity]

    • change: Changed "with your Azure AI Service information" to "with your Azure AI Service credentials"
    • rationale: Using "credentials" is more precise and accurately reflects the security-related information required
    • impact: Reduces ambiguity and clarifies that the constants represent authentication details, aiding correct usage

"> ⚠️ Important:\n",
"You must update the code below to match your Azure authentication method.\n",
"Look for the `# IMPORTANT` comments and modify those sections accordingly.\n",
"If you skip this step, the sample may not run correctly.\n",
"Check for the `# IMPORTANT` comments and modify those sections accordingly.\n",
"If you skip this step, the sample may not run as expected.\n",
"\n",
"> ⚠️ Note: Using a subscription key works, but using a token provider with Azure Active Directory (AAD) is much safer and is highly recommended for production environments."
"> ⚠️ Note:\n",
"Using a subscription key is supported, but using a token provider with Azure Active Directory (AAD) is more secure and strongly recommended for production environments."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Grammar]

    • change: Replaced "Look for" with "Check for" and changed "may not run correctly" to "may not run as expected."
    • rationale: "Check for" is a more precise instruction, and "may not run as expected" is a softer, clearer way of conveying potential issues without implying absolute failure.
    • impact: Enhances the clarity of the instructions and improves the tone to be more informative and less alarming.

  • categories: [Clarity, Grammar, Formatting]

    • change: Split a long note into two lines, changed "Using a subscription key works" to "Using a subscription key is supported," and replaced "much safer and is highly recommended" with "more secure and strongly recommended."
    • rationale: Breaking the note into two lines improves readability and formatting. Refining the wording makes the recommendation clearer and more professional while emphasizing security and best practices in production environments.
    • impact: Improves readability, professionalism, and effectiveness of the cautionary note, making it easier for readers to understand and follow guidance on authentication methods.

},
{
Expand All @@ -52,7 +56,7 @@
"from dotenv import find_dotenv, load_dotenv\n",
"from azure.identity import DefaultAzureCredential, get_bearer_token_provider\n",
"\n",
"# import utility package from python samples root directory\n",
"# Import utility package from Python samples root directory\n",
"parent_dir = Path.cwd().parent\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Grammar, Consistency]
    • change: Capitalized "Import" and "Python" in the comment line.
    • rationale: Starting the comment with a capital letter aligns with standard sentence capitalization rules; capitalizing "Python" ensures proper noun consistency.
    • impact: This improves the professionalism and readability of the comment, making the documentation clearer and more consistent.

"sys.path.append(str(parent_dir))\n",
"from python.content_understanding_face_client import AzureContentUnderstandingFaceClient\n",
Expand All @@ -70,7 +74,7 @@
" token_provider=token_provider,\n",
" # IMPORTANT: Uncomment this if using subscription key\n",
" # subscription_key=os.getenv(\"AZURE_AI_API_KEY\"),\n",
" x_ms_useragent=\"azure-ai-content-understanding-python/build_person_directory\", # This header is used for sample usage telemetry, please comment out this line if you want to opt out.\n",
" x_ms_useragent=\"azure-ai-content-understanding-python/build_person_directory\", # Used for sample usage telemetry; comment out to opt out.\n",
")"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Formatting]
    • change: Reworded the inline comment to be more concise and added a space after the comma before the comment.
    • rationale: The original comment was verbose and used a comma splice; the revised comment is clearer and grammatically correct with proper spacing.
    • impact: Enhances readability and professionalism of the code comments, making the intent easier to understand for developers.

]
},
Expand All @@ -93,43 +97,43 @@
"import uuid\n",
"folder_path = \"../data/face/enrollment_data\" # Replace with the path to your folder containing subfolders of images\n",
"\n",
"# Create a person directory\n",
"# Create a person directory with a unique ID\n",
"person_directory_id = f\"person_directory_id_{uuid.uuid4().hex[:8]}\"\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]
    • change: Updated the comment from "Create a person directory" to "Create a person directory with a unique ID"
    • rationale: The revised comment explicitly states that the directory created includes a unique identifier, reflecting the use of a UUID in the code.
    • impact: This improves the clarity of the code by better describing the purpose and behavior of the operation, helping readers understand that uniqueness is ensured.

"client.create_person_directory(person_directory_id)\n",
"logging.info(f\"Created person directory with ID: {person_directory_id}\")\n",
"\n",
"# Iterate through all subfolders in the folder_path\n",
"# Iterate through all subfolders in the enrollment folder\n",
"for subfolder_name in os.listdir(folder_path):\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]
    • change: Modified the comment from "Iterate through all subfolders in the folder_path" to "Iterate through all subfolders in the enrollment folder"
    • rationale: Replacing the generic term "folder_path" with the more specific "enrollment folder" improves contextual understanding of what the folder contains or represents.
    • impact: Enhances reader comprehension by providing clearer and more meaningful documentation about the code's purpose.

" subfolder_path = os.path.join(folder_path, subfolder_name)\n",
" if os.path.isdir(subfolder_path):\n",
" person_name = subfolder_name\n",
" # Add a person for each subfolder\n",
" # Add a person entry for each subfolder\n",
" person = client.add_person(person_directory_id, tags={\"name\": person_name})\n",
" logging.info(f\"Created person {person_name} with person_id: {person['personId']}\")\n",
" logging.info(f\"Created person '{person_name}' with person_id: {person['personId']}\")\n",
" if person:\n",
" # Iterate through all images in the subfolder\n",
" # Iterate through all image files in the subfolder\n",
" for filename in os.listdir(subfolder_path):\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]

    • change: Modified the comment from "Add a person for each subfolder" to "Add a person entry for each subfolder"
    • rationale: The addition of the word "entry" clarifies that a person record or object is being added, not a physical person.
    • impact: Enhances understanding of what the code is doing, reducing potential ambiguity.

  • categories: [Clarity, Consistency]

    • change: Added single quotes around the {person_name} variable in the logging statement.
    • rationale: Quoting the person name makes it visually distinct in the log output, improving readability and avoiding confusion if the name contains spaces or special characters.
    • impact: Improves log clarity, aiding in easier debugging and monitoring.

  • categories: [Clarity]

    • change: Changed comment from "Iterate through all images in the subfolder" to "Iterate through all image files in the subfolder"
    • rationale: Specifying "image files" clarifies that the iteration targets files representing images rather than other possible meanings of "images".
    • impact: Improves comment accuracy, helping maintainers better understand the code's purpose.

" if filename.lower().endswith(('.png', '.jpg', '.jpeg')):\n",
" image_path = os.path.join(subfolder_path, filename)\n",
" # Convert image to base64\n",
" # Convert image to base64 encoded string\n",
" image_data = AzureContentUnderstandingFaceClient.read_file_to_base64(image_path)\n",
" # Add a face to the Person Directory and associate it to the added person\n",
" # Add the face to the Person Directory and associate it with the person\n",
" face = client.add_face(person_directory_id, image_data, person['personId'])\n",
" if face:\n",
" logging.info(f\"Added face from {filename} with face_id: {face['faceId']} to person_id: {person['personId']}\")\n",
" logging.info(f\"Added face from '{filename}' with face_id: {face['faceId']} to person_id: {person['personId']}\")\n",
" else:\n",
" logging.warning(f\"Failed to add face from {filename} to person_id: {person['personId']}\")\n",
" logging.warning(f\"Failed to add face from '{filename}' to person_id: {person['personId']}\")\n",
"\n",
"logging.info(\"Done\")"
"logging.info(\"Person directory creation complete.\")"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]

    • change: Modified comment from "Convert image to base64" to "Convert image to base64 encoded string"
    • rationale: To provide a more precise description of what the code is doing by specifying that the image is converted to a base64 encoded string rather than just base64
    • impact: Improves the clarity of the comment, making it easier for readers to understand the exact operation

  • categories: [Clarity, Grammar]

    • change: Changed comment from "Add a face to the Person Directory and associate it to the added person" to "Add the face to the Person Directory and associate it with the person"
    • rationale: Enhanced grammatical correctness and used more direct phrasing to reduce ambiguity
    • impact: Provides clearer guidance about the code’s intent and improves readability

  • categories: [Formatting, Consistency]

    • change: Added single quotes around {filename} in logging messages (e.g., from Added face from {filename} to Added face from '{filename}')
    • rationale: To clearly delimit filename values in logs, improving readability and consistency in log formatting
    • impact: Makes log messages easier to parse and visually consistent, aiding in debugging and monitoring

  • categories: [Clarity]

    • change: Updated final log message from logging.info("Done") to logging.info("Person directory creation complete.")
    • rationale: To provide a more descriptive and informative log message indicating the specific operation that has completed
    • impact: Enhances the meaningfulness of the logs, making it easier for users to understand program progress and outcome

},
{
"cell_type": "markdown",
"id": "6a5a058c",
"metadata": {},
"source": [
"### Identifying person\n",
"Detect multiple faces in an image and identify each one by matching it against enrolled persons in the Person Directory."
"### Identifying persons\n",
"Detect multiple faces in an image and identify each by matching against enrolled persons in the Person Directory."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Grammar, Clarity]
    • change: Changed the section title from "Identifying person" to "Identifying persons" and adjusted the sentence to "Detect multiple faces in an image and identify each by matching against enrolled persons in the Person Directory."
    • rationale: Improved grammatical number agreement by using the plural form "persons" to match the context of detecting multiple faces. Simplified the sentence for smoother readability by removing redundant words.
    • impact: Enhances the grammatical accuracy and clarity of the documentation, making the instructions easier to understand for the reader.

},
{
Expand All @@ -144,14 +148,14 @@
"# Detect faces in the test image\n",
"image_data = AzureContentUnderstandingFaceClient.read_file_to_base64(test_image_path)\n",
"detected_faces = client.detect_faces(data=image_data)\n",
"for face in detected_faces['detectedFaces']:\n",
"for face in detected_faces.get('detectedFaces', []):\n",
" identified_persons = client.identify_person(person_directory_id, image_data, face['boundingBox'])\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Consistency]
    • change: Replaced direct dictionary key access detected_faces['detectedFaces'] with the safer .get('detectedFaces', []) method.
    • rationale: Using .get() with a default empty list prevents potential KeyError exceptions if the key 'detectedFaces' is missing, making the code more robust.
    • impact: Enhances code reliability by handling missing keys gracefully, preventing runtime errors and improving maintainability.

" if identified_persons.get(\"personCandidates\"):\n",
" person = identified_persons[\"personCandidates\"][0]\n",
" name = person.get(\"tags\", {}).get(\"name\", \"Unknown\")\n",
" logging.info(f\"Detected person: {name} with confidence: {person.get('confidence', 0)} at bounding box: {face['boundingBox']}\")\n",
" logging.info(f\"Detected person: {name} with confidence: {person.get('confidence', 0):.2f} at bounding box: {face['boundingBox']}\")\n",
"\n",
"logging.info(\"Done\")"
"logging.info(\"Identification complete.\")"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Formatting, Clarity]

    • change: Added numerical formatting :.2f to the confidence value in the logging message.
    • rationale: Limits the displayed confidence to two decimal places for better readability and consistent presentation of numerical data.
    • impact: Improves clarity of logged information by avoiding excessively long or unwieldy floating-point numbers.

  • categories: [Clarity]

    • change: Changed the final logging message from "Done" to "Identification complete."
    • rationale: Provides a more descriptive and meaningful message indicating what process has completed.
    • impact: Enhances understanding of log output by clearly stating the completion of the identification stage.

},
{
Expand All @@ -160,7 +164,7 @@
"metadata": {},
"source": [
"### Adding and associating a new face\n",
"You can add a new face to the Person Directory and associate it with an existing person."
"You can add a new face image to the Person Directory and associate it with an existing person."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]
    • change: Added the word "image" after "face" in the sentence.
    • rationale: To specify that the new face being added is an image, making the instruction more precise.
    • impact: Enhances the clarity of the documentation by clearly indicating that a face image is being added, reducing potential ambiguity.

},
{
Expand All @@ -170,17 +174,17 @@
"metadata": {},
"outputs": [],
"source": [
"new_face_image_path = \"new_face_image_path\" # The path to the face image you want to add.\n",
"existing_person_id = \"existing_person_id\" # The unique ID of the person to whom the face should be associated.\n",
"new_face_image_path = \"new_face_image_path\" # Path to the new face image to be added.\n",
"existing_person_id = \"existing_person_id\" # Unique ID of the person to associate the face with.\n",
"\n",
"# Convert the new face image to base64\n",
"# Convert the new face image to a base64 encoded string\n",
"image_data = AzureContentUnderstandingFaceClient.read_file_to_base64(new_face_image_path)\n",
"# Add the new face to the person directory and associate it with the existing person\n",
"# Add the new face to the Person Directory and associate it with the existing person\n",
"face = client.add_face(person_directory_id, image_data, existing_person_id)\n",
"if face:\n",
" logging.info(f\"Added face from {new_face_image_path} with face_id: {face['faceId']} to person_id: {existing_person_id}\")\n",
" logging.info(f\"Added face from '{new_face_image_path}' with face_id: {face['faceId']} to person_id: {existing_person_id}\")\n",
"else:\n",
" logging.warning(f\"Failed to add face from {new_face_image_path} to person_id: {existing_person_id}\")"
" logging.warning(f\"Failed to add face from '{new_face_image_path}' to person_id: {existing_person_id}\")"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Consistency, Formatting]

    • change: Updated inline comments to be more descriptive and consistent, e.g., changing "The path to the face image you want to add." to "Path to the new face image to be added." and "Convert the new face image to base64" to "Convert the new face image to a base64 encoded string."
    • rationale: More precise wording clarifies the purpose of variables and actions, enhancing reader understanding and maintaining consistent style throughout the comments.
    • impact: Improves readability and comprehension of the code for developers and maintainers.

  • categories: [Consistency, Formatting]

    • change: Capitalized "Person Directory" consistently in the comments.
    • rationale: Ensures consistent naming conventions for important entities, which helps in emphasizing their significance and avoiding confusion.
    • impact: Contributes to a professional and uniform codebase.

  • categories: [Formatting, Clarity]

    • change: Added single quotes around the variable {new_face_image_path} in logging messages.
    • rationale: Quoting the variable output improves log clarity by visually delimiting the filename/path, making log messages easier to read and parse.
    • impact: Enhances the quality and usefulness of log entries for debugging and monitoring.

},
{
Expand All @@ -190,7 +194,7 @@
"source": [
"### Associating a list of already enrolled faces\n",
"\n",
"You can associate a list of already enrolled faces in the Person Directory with their respective persons. This is useful if you have existing face IDs to link to specific persons."
"You can associate a list of existing face IDs with their respective persons in the Person Directory. This is useful when linking pre-enrolled faces to specific persons."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Grammar]
    • change: Rephrased the sentence to "You can associate a list of existing face IDs with their respective persons in the Person Directory. This is useful when linking pre-enrolled faces to specific persons."
    • rationale: The revised wording is more concise and eliminates redundancy ("already enrolled faces" to "existing face IDs") while improving readability. It also clarifies the context by using "linking pre-enrolled faces" which is more precise.
    • impact: This change enhances the clarity and professional tone of the documentation, making it easier for readers to understand the purpose and usage of the feature.

},
{
Expand All @@ -200,10 +204,10 @@
"metadata": {},
"outputs": [],
"source": [
"existing_person_id = \"existing_person_id\" # The unique ID of the person to whom the face should be associated.\n",
"existing_face_id_list = [\"existing_face_id_1\", \"existing_face_id_2\"] # The list of face IDs to be associated.\n",
"existing_person_id = \"existing_person_id\" # Unique ID of the person to associate faces with.\n",
"existing_face_id_list = [\"existing_face_id_1\", \"existing_face_id_2\"] # List of face IDs to associate.\n",
"\n",
"# Associate the existing face IDs with the existing person\n",
"# Associate the listed face IDs with the specified person\n",
"client.update_person(person_directory_id, existing_person_id, face_ids=existing_face_id_list)"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]

    • change: Revised inline comments for variable definitions to use more concise and direct wording.
    • rationale: The original comments were slightly verbose; the new comments express the same meaning more succinctly.
    • impact: Enhances readability and clarity of the variable purpose without losing context.

  • categories: [Clarity]

    • change: Modified the block comment describing the code action for associating face IDs with a person.
    • rationale: The updated comment specifies "listed face IDs" and "specified person," which more clearly describes the operation performed.
    • impact: Provides a clearer explanation of the code’s intent, improving understanding for future readers or maintainers.

]
},
Expand All @@ -213,7 +217,7 @@
"metadata": {},
"source": [
"### Associating and disassociating a face from a person\n",
"You can associate or disassociate a face from a person in the Person Directory. Associating a face links it to a specific person, while disassociating removes this link."
"You can link a face to a person or remove such a link in the Person Directory. Associating a face connects it to a specific person, while disassociating removes this connection."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Consistency]
    • change: Reworded the sentence from "You can associate or disassociate a face from a person..." to "You can link a face to a person or remove such a link..." and replaced "Associating a face links it" with "Associating a face connects it"; similarly changed "disassociating removes this link" to "disassociating removes this connection."
    • rationale: The revised wording uses simpler and more consistent terms ("link" and "connection") to clearly convey the relationship between a face and a person, avoiding repetition of similar words and improving sentence flow.
    • impact: Enhances readability and understanding for users by using clear, consistent terminology describing the association between a face and a person.

},
{
Expand All @@ -223,18 +227,18 @@
"metadata": {},
"outputs": [],
"source": [
"existing_face_id = \"existing_face_id\" # The unique ID of the face.\n",
"existing_face_id = \"existing_face_id\" # Unique ID of the face.\n",
"\n",
"# Remove the association of the existing face ID from the person\n",
"client.update_face(person_directory_id, existing_face_id, person_id=\"\") # The person_id is set to \"\" to remove the association\n",
"logging.info(f\"Removed association of face_id: {existing_face_id} from the existing person_id\")\n",
"logging.info(client.get_face(person_directory_id, existing_face_id)) # This will return the face information without the person association\n",
"# Remove the association of the face from its person by setting person_id to an empty string\n",
"client.update_face(person_directory_id, existing_face_id, person_id=\"\")\n",
"logging.info(f\"Removed association of face_id: {existing_face_id} from its person.\")\n",
"logging.info(client.get_face(person_directory_id, existing_face_id)) # Returns face information without person association\n",
"\n",
"# Associate the existing face ID with a person\n",
"existing_person_id = \"existing_person_id\" # The unique ID of the person to be associated with the face.\n",
"# Associate the face ID with a person\n",
"existing_person_id = \"existing_person_id\" # Unique ID of the person to associate the face with.\n",
"client.update_face(person_directory_id, existing_face_id, person_id=existing_person_id)\n",
"logging.info(f\"Associated face_id: {existing_face_id} with person_id: {existing_person_id}\")\n",
"logging.info(client.get_face(person_directory_id, existing_face_id)) # This will return the face information with the new person association"
"logging.info(client.get_face(person_directory_id, existing_face_id)) # Returns face information with updated person association"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Grammar]

    • change: Changed comment from "The unique ID of the face." to "Unique ID of the face."
    • rationale: Removed unnecessary article to make the comment more concise.
    • impact: Improves readability and conciseness of the code comment.

  • categories: [Clarity, Grammar]

    • change: Revised comment from "# Remove the association of the existing face ID from the person" to "# Remove the association of the face from its person by setting person_id to an empty string" and removed inline explanatory comment.
    • rationale: Provides a clearer explanation of what setting person_id="" does, making the intent explicit.
    • impact: Enhances understanding of the code’s purpose and how the operation is performed.

  • categories: [Clarity, Formatting]

    • change: Adjusted the logging message to "Removed association of face_id: {existing_face_id} from its person." and removed an inline comment.
    • rationale: Streamlines the message and removes redundancy.
    • impact: Improves log clarity and readability.

  • categories: [Clarity, Grammar, Formatting]

    • change: Modified logging comment to "Returns face information without person association" from a longer description.
    • rationale: Simplifies and clarifies the comment while maintaining information.
    • impact: Makes the comment more concise and easier to read.

  • categories: [Clarity, Grammar]

    • change: Changed comment from "# Associate the existing face ID with a person" to "# Associate the face ID with a person" and modified the inline comment to "Unique ID of the person to associate the face with."
    • rationale: Removes the word "existing" to avoid redundancy and improves phrasing.
    • impact: Enhances clarity and conciseness of the comment.

  • categories: [Clarity, Formatting]

    • change: Updated final logging comment to "Returns face information with updated person association" from a longer form.
    • rationale: Shortened and clarified the comment for better readability.
    • impact: Makes the comment straightforward and easier to comprehend.

},
{
Expand All @@ -243,7 +247,7 @@
"metadata": {},
"source": [
"### Updating metadata (tags and descriptions)\n",
"You can add or update tags for individual persons, and both descriptions and tags for the Person Directory. These metadata fields help organize, filter, and manage your directory."
"You can add or update tags for individual persons as well as update the description and tags for the entire Person Directory. These metadata fields help organize, filter, and manage your directory effectively."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Grammar]
    • change: Reworded the sentence to clarify that tags can be added or updated both for individual persons and for the entire Person Directory description and tags.
    • rationale: The original phrasing was slightly ambiguous; the revision separates actions on individual persons from actions on the entire directory more clearly and improves sentence flow.
    • impact: Improves reader understanding by clearly distinguishing the scope of metadata updates and enhances the professional tone of the documentation.

},
{
Expand All @@ -263,19 +267,19 @@
" tags=person_directory_tags\n",
")\n",
"logging.info(f\"Updated Person Directory with description: '{person_directory_description}' and tags: {person_directory_tags}\")\n",
"logging.info(client.get_person_directory(person_directory_id)) # This will return the updated person directory information\n",
"logging.info(client.get_person_directory(person_directory_id)) # Returns the updated Person Directory information\n",
"\n",
"# Update the tags for an individual person\n",
"existing_person_id = \"existing_person_id\" # The unique ID of the person to update.\n",
"person_tags = {\"role\": \"tester\", \"department\": \"engineering\", \"name\": \"\"} # This will remove the name tag from the person.\n",
"existing_person_id = \"existing_person_id\" # Unique ID of the person to update\n",
"person_tags = {\"role\": \"tester\", \"department\": \"engineering\", \"name\": \"\"} # This will remove the 'name' tag from the person\n",
"\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Formatting, Clarity]

    • change: Added an extra space before the inline comment in the logging.info line and rephrased the comment to "Returns the updated Person Directory information"
    • rationale: To improve readability by properly spacing the comment and making the comment more concise and clearer
    • impact: Enhances readability and professionalism of the code comments

  • categories: [Grammar, Consistency, Clarity]

    • change: Removed the article "The" from the inline comment about existing_person_id, changing "The unique ID of the person to update." to "Unique ID of the person to update"
    • rationale: To maintain consistency with other comments and make the phrasing more direct and concise
    • impact: Produces uniformity in comment style and improves clarity

  • categories: [Formatting, Clarity]

    • change: Added a space before the inline comment on the person_tags line and enclosed the 'name' tag in single quotes within the comment
    • rationale: Adding space enhances readability; enclosing 'name' in quotes clarifies that it is a specific tag key being referenced
    • impact: Improves readability and helps developers quickly identify the key subject of the comment

"client.update_person(\n",
" person_directory_id,\n",
" existing_person_id,\n",
" tags=person_tags\n",
")\n",
"logging.info(f\"Updated person with person_id: {existing_person_id} with tags: {person_tags}\")\n",
"logging.info(client.get_person(person_directory_id, existing_person_id)) # This will return the updated person information"
"logging.info(client.get_person(person_directory_id, existing_person_id)) # Returns the updated person information"
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Grammar, Clarity]
    • change: Removed "This will" and changed the sentence to "Returns the updated person information" in the inline comment.
    • rationale: Simplified the comment to a more direct and assertive statement, improving readability and clarity.
    • impact: Makes the comment easier to understand and more concise, enhancing the overall quality of the documentation within the code.

},
{
Expand All @@ -284,7 +288,7 @@
"metadata": {},
"source": [
"### Deleting a face\n",
"You can also delete a specific face. Once the face is deleted, the association between the face and its associated person is removed."
"You can delete a specific face from the Person Directory. Once deleted, the association between the face and the person is removed."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Consistency]
    • change: Modified the sentence to specify "delete a specific face from the Person Directory" and rephrased the second sentence for conciseness.
    • rationale: Adding "from the Person Directory" clarifies where the action takes place, and rephrasing improves the flow and removes redundancy regarding the association removal.
    • impact: The documentation becomes clearer and more precise, making it easier for users to understand the operation and its effects.

},
{
Expand All @@ -294,7 +298,7 @@
"metadata": {},
"outputs": [],
"source": [
"existing_face_id = \"existing_face_id\" # The unique ID of the face to delete.\n",
"existing_face_id = \"existing_face_id\" # Unique ID of the face to delete\n",
"\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Grammar, Formatting]
    • change: Removed the definite article "The" at the beginning of the comment and the trailing period; added a space before the inline comment.
    • rationale: The comment was revised to be more concise and follow common Python inline comment formatting conventions, improving readability.
    • impact: Enhances clarity and consistency of the inline comment, making the code easier to read and understand.

"client.delete_face(person_directory_id, existing_face_id)\n",
"logging.info(f\"Deleted face with face_id: {existing_face_id}\")"
Expand All @@ -307,7 +311,7 @@
"source": [
"### Deleting a person\n",
"\n",
"When a person is deleted from the Person Directory, all the faces associated with that person remain in the Person Directory, but the association between the person and the faces is removed. This means the faces are no longer associated to any person in the Person Directory."
"When a person is deleted from the Person Directory, all faces associated with that person remain in the directory but their association with the person is removed. This means the faces are no longer linked to any person."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Grammar]
    • change: Simplified the sentence structure by removing redundant phrases such as "person in the Person Directory" and rephrasing for conciseness.
    • rationale: To enhance readability and make the explanation more straightforward by eliminating unnecessary repetition.
    • impact: The resulting text is clearer and more concise, improving user understanding of the behavior when a person is deleted from the directory.

},
{
Expand All @@ -317,7 +321,7 @@
"metadata": {},
"outputs": [],
"source": [
"existing_person_id = \"existing_person_id\" # The unique ID of the person to delete.\n",
"existing_person_id = \"existing_person_id\" # Unique ID of the person to delete\n",
"\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Grammar, Clarity]
    • change: Removed the definite article "The" at the beginning of the inline comment and the trailing period at the end.
    • rationale: Simplified the comment for more concise and direct wording, which improves readability and maintains a consistent style with other comments.
    • impact: Makes the inline comment clearer and easier to read without extraneous words or punctuation.

"client.delete_person(person_directory_id, existing_person_id)\n",
"logging.info(f\"Deleted person with person_id: {existing_person_id}\")"
Expand All @@ -330,7 +334,7 @@
"source": [
"### Deleting a person and their associated faces\n",
"\n",
"To completely remove a person and all their associated faces from the Person Directory, you can delete the person along with their face associations. This operation ensures that no residual data related to the person remains in the directory."
"To completely remove a person and all their associated faces from the Person Directory, delete the faces first, then delete the person. This ensures no residual data remains linked to that person."
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]
    • change: Reworded the instruction from a passive construction ("you can delete the person along with their face associations") to a clear procedural sequence ("delete the faces first, then delete the person").
    • rationale: This change clarifies the correct order of operations to ensure proper deletion without residual data.
    • impact: Enhances user understanding by providing a precise, actionable sequence, reducing potential mistakes during deletion.

},
{
Expand All @@ -340,15 +344,15 @@
"metadata": {},
"outputs": [],
"source": [
"existing_person_id = \"existing_person_id\" # The unique ID of the person to delete.\n",
"existing_person_id = \"existing_person_id\" # Unique ID of the person to delete\n",
"\n",
"# Get the list of face IDs associated with the person\n",
"# Retrieve the list of face IDs associated with the person\n",
"response = client.get_person(person_directory_id, existing_person_id)\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity, Consistency]

    • change: Removed "The" from the comment describing existing_person_id and removed the trailing period.
    • rationale: Simplifies the comment for a more direct description and aligns with a style that omits periods in inline comments.
    • impact: Enhances readability and maintains consistent comment style across the code.

  • categories: [Clarity]

    • change: Changed comment from "Get the list of face IDs associated with the person" to "Retrieve the list of face IDs associated with the person".
    • rationale: "Retrieve" is more formal and precise than "Get," improving clarity.
    • impact: Provides a clearer and more professional tone to the documentation comment.

"face_ids = response.get('faceIds', [])\n",
"\n",
"# Delete each face associated with the person\n",
"for face_id in face_ids:\n",
" logging.info(f\"Deleting face with face_id: {face_id} from person_id: {existing_person_id}\")\n",
" logging.info(f\"Deleting face with face_id: {face_id} associated with person_id: {existing_person_id}\")\n",
" client.delete_face(person_directory_id, face_id)\n",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Clarity]
    • change: Changed the log message from "Deleting face with face_id: ... from person_id: ..." to "Deleting face with face_id: ... associated with person_id: ..."
    • rationale: The word "associated with" better clarifies the relationship between the face and the person, avoiding potential ambiguity that "from" might cause.
    • impact: Improves the clarity of log messages, making it easier for developers or users to understand the exact action being performed.

"\n",
"# Delete the person after deleting all associated faces\n",
Expand Down Expand Up @@ -378,4 +382,4 @@
},
"nbformat": 4,
"nbformat_minor": 5
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • categories: [Formatting]
    • change: Added a newline after the closing brace }.
    • rationale: Ensuring the file ends with a newline to adhere to common coding standards and avoid potential issues with some tools that expect a trailing newline.
    • impact: Improves compatibility with development tools and consistent formatting across files.