-
Notifications
You must be signed in to change notification settings - Fork 516
Improve Face Clustering Accuracy & Enable Automatic Clustering on Folder Add #914
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,8 @@ def post_folder_add_sequence(folder_path: str, folder_id: int): | |
| logger.info(f"Add folder: {folder_data}") | ||
| # Process images in all folders | ||
| image_util_process_folder_images(folder_data) | ||
| image_util_process_untagged_images() | ||
| cluster_util_face_clusters_sync(force_full_reclustering=True) # Force full reclustering for new photos | ||
|
Comment on lines
+72
to
+73
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check for performance-related configuration options or limits for clustering
rg -n -C3 'force_full_reclustering|cluster.*config|clustering.*threshold|max.*faces|performance' --type=pyRepository: AOSSIE-Org/PictoPy Length of output: 3064 🏁 Script executed: #!/bin/bash
# Find the implementation of cluster_util_face_clusters_sync to understand actual behavior
rg -n 'def cluster_util_face_clusters_sync|def face_clusters_sync' --type=py -A 15Repository: AOSSIE-Org/PictoPy Length of output: 1290 🏁 Script executed: #!/bin/bash
# Check all usages of force_full_reclustering to understand the pattern
rg -n 'force_full_reclustering' --type=py -B 2 -A 2Repository: AOSSIE-Org/PictoPy Length of output: 2316 🏁 Script executed: #!/bin/bash
# Get the implementation of cluster_util_cluster_all_face_embeddings
rg -n 'def cluster_util_cluster_all_face_embeddings' --type=py -A 20Repository: AOSSIE-Org/PictoPy Length of output: 1774 🏁 Script executed: #!/bin/bash
# Get the implementation of cluster_util_is_reclustering_needed to understand the 24-hour rule
rg -n 'def cluster_util_is_reclustering_needed' --type=py -A 15Repository: AOSSIE-Org/PictoPy Length of output: 1210 🏁 Script executed: #!/bin/bash
# Search for any documentation about performance or the behavior of full reclustering
rg -n 'reclustering|cluster.*all.*faces|cluster_all' --type=py | head -30Repository: AOSSIE-Org/PictoPy Length of output: 3115 Misleading comment: Full reclustering processes all faces, not just new photos. The comment "Force full reclustering for new photos" is inaccurate. The Additionally, forcing full reclustering on every folder add and sync (lines 73 and 122) bypasses the 24-hour throttling mechanism in Consider either:
🤖 Prompt for AI Agents |
||
|
|
||
| # Restart sync microservice watcher after processing images | ||
| API_util_restart_sync_microservice_watcher() | ||
|
|
@@ -117,7 +119,7 @@ def post_sync_folder_sequence( | |
| # Process images in all folders | ||
| image_util_process_folder_images(folder_data) | ||
| image_util_process_untagged_images() | ||
| cluster_util_face_clusters_sync() | ||
| cluster_util_face_clusters_sync(force_full_reclustering=True) # Force full reclustering for synced photos | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same performance concern applies to sync operations. This has the same issue as lines 72-73: full reclustering processes all photos in the database, not just newly synced ones. The comment is similarly misleading, and the performance impact could be significant for large libraries during routine sync operations. 🤖 Prompt for AI Agents |
||
|
|
||
| # Restart sync microservice watcher after processing images | ||
| API_util_restart_sync_microservice_watcher() | ||
|
|
@@ -184,7 +186,7 @@ def add_folder(request: AddFolderRequest, app_state=Depends(get_state)): | |
| root_folder_id, folder_map = folder_util_add_folder_tree( | ||
| root_path=request.folder_path, | ||
| parent_folder_id=parent_folder_id, | ||
| AI_Tagging=False, | ||
| AI_Tagging=True, # Enable AI tagging by default for automatic face detection | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cd /root/repo && cat -n backend/app/routes/folders.py | sed -n '180,200p'Repository: AOSSIE-Org/PictoPy Length of output: 122 🏁 Script executed: cd /root/repo && git log --oneline -20 -- backend/app/routes/folders.py | head -5Repository: AOSSIE-Org/PictoPy Length of output: 122 🏁 Script executed: cd /root/repo && rg "AI_Tagging" -B 3 -A 3 --type pyRepository: AOSSIE-Org/PictoPy Length of output: 122 🏁 Script executed: cd /root/repo && fd -t f "README\|CHANGELOG\|CHANGES\|RELEASE" -i | head -10Repository: AOSSIE-Org/PictoPy Length of output: 122 🏁 Script executed: cd /root/repo && rg "face.*detect\|automatic.*process" -i --type py | head -20Repository: AOSSIE-Org/PictoPy Length of output: 122 🌐 Web query:
💡 Result: Short best-practices checklist for automatic face-detection/recognition (photo management) and user consent:
If you want, I can: draft UI consent text, a minimal privacy notice, or a retention-policy template that follows these points. 🏁 Script executed: pwd && git ls-files | grep -E "folders\.py|README|CHANGELOG" | head -20Repository: AOSSIE-Org/PictoPy Length of output: 491 🏁 Script executed: cat backend/app/routes/folders.py | sed -n '180,200p'Repository: AOSSIE-Org/PictoPy Length of output: 1033 🏁 Script executed: rg "AI_Tagging" -B 3 -A 3 --type pyRepository: AOSSIE-Org/PictoPy Length of output: 9497 🏁 Script executed: git log --oneline -p -- backend/app/routes/folders.py | grep -A 10 -B 10 "AI_Tagging" | head -40Repository: AOSSIE-Org/PictoPy Length of output: 1703 🏁 Script executed: rg "add_folder|Folder.*AI" -B 2 -A 2 --type py | head -50Repository: AOSSIE-Org/PictoPy Length of output: 3107 🏁 Script executed: find . -type f \( -name "*.md" -o -name "*.rst" -o -name "*.txt" \) | xargs grep -l "AI\|tagging\|face.*detect" 2>/dev/null | head -10Repository: AOSSIE-Org/PictoPy Length of output: 285 🏁 Script executed: rg "automatic.*face\|AI.*tag\|face.*detect" -i --type md | head -30Repository: AOSSIE-Org/PictoPy Length of output: 44 🏁 Script executed: cat README.mdRepository: AOSSIE-Org/PictoPy Length of output: 2620 🏁 Script executed: rg "consent\|privacy\|opt.*in\|opt.*out" -i --type py --type md | head -30Repository: AOSSIE-Org/PictoPy Length of output: 44 🏁 Script executed: find . -path ./node_modules -prune -o -type f -name "*.json" -o -name "*.tsx" -o -name "*.ts" | xargs grep -l "AI\|tagging\|consent" 2>/dev/null | head -10Repository: AOSSIE-Org/PictoPy Length of output: 516 🏁 Script executed: rg "AddFolder|add_folder" -A 5 --type py | grep -i "request\|response" | head -20Repository: AOSSIE-Org/PictoPy Length of output: 1365 🏁 Script executed: cat frontend/src/components/FolderPicker/AITaggingFolderPicker.tsxRepository: AOSSIE-Org/PictoPy Length of output: 1464 🏁 Script executed: cat frontend/src/components/EmptyStates/EmptyAITaggingState.tsxRepository: AOSSIE-Org/PictoPy Length of output: 1254 🏁 Script executed: cat backend/app/schemas/folders.py | grep -A 10 "class AddFolderRequest"Repository: AOSSIE-Org/PictoPy Length of output: 357 🏁 Script executed: rg "AddFolderRequest" -B 2 -A 5 frontend/srcRepository: AOSSIE-Org/PictoPy Length of output: 1071 🏁 Script executed: cat docs/backend/backend_python/image-processing.mdRepository: AOSSIE-Org/PictoPy Length of output: 5247 Remove AI_Tagging=True default or implement explicit user consent and control. Automatic face detection on all newly added folders violates privacy best practices and contradicts the UI, which states "Enable AI Tagging for a folder from the settings page." The API forces AI_Tagging=True with no user control:
Either:
Document the change in release notes and clarify the UI messaging to reflect actual behavior. 🤖 Prompt for AI Agents |
||
| taggingCompleted=request.taggingCompleted, | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| """ | ||
| Face alignment utilities for improving face recognition accuracy. | ||
|
|
||
| This module provides face alignment preprocessing to handle tilted, angled, | ||
| and profile faces. It uses simple geometric transformation based on eye positions. | ||
| """ | ||
|
|
||
| import cv2 | ||
| import numpy as np | ||
| from typing import Optional, Tuple, Dict | ||
| from app.config.settings import FACE_ALIGNMENT_ENABLED | ||
| from app.logging.setup_logging import get_logger | ||
|
|
||
| logger = get_logger(__name__) | ||
|
|
||
|
|
||
| def estimate_eye_positions(face_image: np.ndarray) -> Optional[Tuple[Tuple[int, int], Tuple[int, int]]]: | ||
| """ | ||
| Estimate eye positions using simple heuristics for face crops. | ||
|
|
||
| For a cropped face image, eyes are typically: | ||
| - Horizontally: At 1/4 and 3/4 of width | ||
| - Vertically: At 1/3 of height from top | ||
|
|
||
| Args: | ||
| face_image: Cropped face image | ||
|
|
||
| Returns: | ||
| Tuple of (left_eye, right_eye) coordinates, or None if estimation fails | ||
| """ | ||
| h, w = face_image.shape[:2] | ||
|
|
||
| # Heuristic eye positions (works reasonably well for frontal faces) | ||
| left_eye = (int(w * 0.35), int(h * 0.35)) | ||
| right_eye = (int(w * 0.65), int(h * 0.35)) | ||
|
|
||
| return (left_eye, right_eye) | ||
|
|
||
|
|
||
| def align_face_simple( | ||
| image: np.ndarray, | ||
| bbox: Dict[str, int], | ||
| padding: int = 20 | ||
| ) -> np.ndarray: | ||
| """ | ||
| Extract and align face using simple geometric heuristics. | ||
|
|
||
| This is a lightweight alignment approach that: | ||
| 1. Crops the face with padding | ||
| 2. Estimates eye positions using heuristics | ||
| 3. Rotates face to align eyes horizontally | ||
| 4. Returns aligned face crop | ||
|
|
||
| Args: | ||
| image: Full source image | ||
| bbox: Bounding box dict with keys: x, y, width, height | ||
| padding: Padding around face in pixels | ||
|
|
||
| Returns: | ||
| Aligned face crop as numpy array | ||
| """ | ||
| if not FACE_ALIGNMENT_ENABLED: | ||
| # Fallback to simple crop when alignment disabled | ||
| return simple_face_crop(image, bbox, padding) | ||
|
|
||
| try: | ||
| # Extract face region with padding | ||
| x, y, w, h = bbox['x'], bbox['y'], bbox['width'], bbox['height'] | ||
| img_h, img_w = image.shape[:2] | ||
|
|
||
| # Calculate crop bounds with padding | ||
| x1 = max(0, x - padding) | ||
| y1 = max(0, y - padding) | ||
| x2 = min(img_w, x + w + padding) | ||
| y2 = min(img_h, y + h + padding) | ||
|
|
||
| # Crop face region | ||
| face_crop = image[y1:y2, x1:x2] | ||
|
|
||
| if face_crop.size == 0: | ||
| logger.warning("Empty face crop, returning original") | ||
| return simple_face_crop(image, bbox, padding) | ||
|
|
||
| # Estimate eye positions | ||
| eyes = estimate_eye_positions(face_crop) | ||
| if eyes is None: | ||
| return face_crop | ||
|
|
||
| left_eye, right_eye = eyes | ||
|
|
||
| # Calculate rotation angle to align eyes horizontally | ||
| dx = right_eye[0] - left_eye[0] | ||
| dy = right_eye[1] - left_eye[1] | ||
| angle = np.degrees(np.arctan2(dy, dx)) | ||
|
|
||
| # Only apply rotation if angle is significant (> 3 degrees) | ||
| if abs(angle) < 3: | ||
| return face_crop | ||
|
|
||
| # Calculate center point for rotation (between eyes) | ||
| center_x = (left_eye[0] + right_eye[0]) // 2 | ||
| center_y = (left_eye[1] + right_eye[1]) // 2 | ||
| center = (center_x, center_y) | ||
|
|
||
| # Create rotation matrix | ||
| M = cv2.getRotationMatrix2D(center, angle, scale=1.0) | ||
|
|
||
| # Apply rotation | ||
| rotated = cv2.warpAffine( | ||
| face_crop, | ||
| M, | ||
| (face_crop.shape[1], face_crop.shape[0]), | ||
| flags=cv2.INTER_LINEAR, | ||
| borderMode=cv2.BORDER_REPLICATE | ||
| ) | ||
|
|
||
| logger.debug(f"Aligned face with rotation angle: {angle:.2f} degrees") | ||
| return rotated | ||
|
|
||
| except Exception as e: | ||
| logger.warning(f"Face alignment failed: {e}, using simple crop") | ||
| return simple_face_crop(image, bbox, padding) | ||
|
|
||
|
|
||
| def simple_face_crop( | ||
| image: np.ndarray, | ||
| bbox: Dict[str, int], | ||
| padding: int = 20 | ||
| ) -> np.ndarray: | ||
| """ | ||
| Simple face crop without alignment (fallback method). | ||
|
|
||
| Args: | ||
| image: Full source image | ||
| bbox: Bounding box dict with keys: x, y, width, height | ||
| padding: Padding around face in pixels | ||
|
|
||
| Returns: | ||
| Face crop as numpy array | ||
| """ | ||
| x, y, w, h = bbox['x'], bbox['y'], bbox['width'], bbox['height'] | ||
| img_h, img_w = image.shape[:2] | ||
|
|
||
| x1 = max(0, x - padding) | ||
| y1 = max(0, y - padding) | ||
| x2 = min(img_w, x + w + padding) | ||
| y2 = min(img_h, y + h + padding) | ||
|
|
||
| return image[y1:y2, x1:x2] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -186,19 +186,19 @@ def _validate_embedding(embedding: NDArray, min_norm: float = 1e-6) -> bool: | |
|
|
||
|
|
||
| def cluster_util_cluster_all_face_embeddings( | ||
| eps: float = 0.75, | ||
| eps: float = 0.5, | ||
| min_samples: int = 2, | ||
| similarity_threshold: float = 0.85, | ||
| similarity_threshold: float = 0.65, | ||
| merge_threshold: float = None, | ||
| ) -> List[ClusterResult]: | ||
| """ | ||
| Cluster face embeddings using DBSCAN with similarity validation. | ||
|
|
||
| Args: | ||
| eps: DBSCAN epsilon parameter for maximum distance between samples (default: 0.75) | ||
| eps: DBSCAN epsilon parameter for maximum distance between samples (default: 0.5) | ||
| min_samples: DBSCAN minimum samples parameter for core points (default: 2) | ||
| similarity_threshold: Minimum similarity to consider same person (default: 0.85, range: 0.75-0.90) | ||
| merge_threshold: Similarity threshold for post-clustering merge (default: None, uses similarity_threshold) | ||
| similarity_threshold: Minimum similarity to consider same person (default: 0.65, range: 0.60-0.85) | ||
| merge_threshold: Similarity threshold for post-clustering merge (default: None, uses 0.60) | ||
|
|
||
| Returns: | ||
| List of ClusterResult objects containing face_id, embedding, cluster_uuid, and cluster_name | ||
|
|
@@ -306,8 +306,8 @@ def cluster_util_cluster_all_face_embeddings( | |
| results.append(result) | ||
|
|
||
| # Post-clustering merge: merge similar clusters based on representative faces | ||
| # Use similarity_threshold if merge_threshold not explicitly provided | ||
| effective_merge_threshold = merge_threshold if merge_threshold is not None else 0.7 | ||
| # Use lower threshold if merge_threshold not explicitly provided to combine similar clusters | ||
| effective_merge_threshold = merge_threshold if merge_threshold is not None else 0.60 | ||
| results = _merge_similar_clusters( | ||
| results, merge_threshold=effective_merge_threshold | ||
| ) | ||
|
Comment on lines
+309
to
313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major The merge threshold of 0.60 is very aggressive. Clusters with only 60% cosine similarity will be merged together. This is quite low and risks combining distinct individuals, especially since it's lower than the primary Consider increasing the default to at least match or exceed 🤖 Prompt for AI Agents |
||
|
|
@@ -316,7 +316,7 @@ def cluster_util_cluster_all_face_embeddings( | |
|
|
||
|
|
||
| def cluster_util_assign_cluster_to_faces_without_clusterId( | ||
| similarity_threshold: float = 0.8, | ||
| similarity_threshold: float = 0.65, | ||
| ) -> List[Dict]: | ||
| """ | ||
| Assign cluster IDs to faces that don't have clusters using nearest mean method with similarity threshold. | ||
|
|
@@ -331,7 +331,7 @@ def cluster_util_assign_cluster_to_faces_without_clusterId( | |
| Args: | ||
| similarity_threshold: | ||
| Minimum cosine similarity required for assignment (0.0 to 1.0) | ||
| Higher values = more strict assignment. Default: 0.7 | ||
| Higher values = more strict assignment. Default: 0.65 | ||
|
|
||
| Returns: | ||
| List of face-cluster mappings ready for batch update | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Move the import to module level.
The import statement is currently inside the loop, which means it will be executed for every face detected. This is inefficient and goes against Python best practices.
🔎 Proposed fix
Move the import to the top of the file with other imports:
from app.models.FaceNet import FaceNet from app.utils.FaceNet import FaceNet_util_preprocess_image, FaceNet_util_get_model_path from app.utils.YOLO import YOLO_util_get_model_path from app.models.YOLO import YOLO from app.database.faces import db_insert_face_embeddings_by_image_id from app.logging.setup_logging import get_logger +from app.utils.face_alignment import align_face_simpleThen remove it from the loop:
📝 Committable suggestion
🤖 Prompt for AI Agents