[draft] add optional return_2d flag for including 2D results in predict() return#21
[draft] add optional return_2d flag for including 2D results in predict() return#21deruyter92 wants to merge 1 commit intomainfrom
return_2d flag for including 2D results in predict() return#21Conversation
- added a field pose_2d to Pose3DResult. - predict populates the field with the 2D pose estimation if return_2d=True
There was a problem hiding this comment.
Pull request overview
Adds an optional return_2d convenience flag to the inference API so callers of FMPose3DInference.predict() can retrieve the intermediate 2D estimation alongside the final 3D result.
Changes:
- Added
pose_2d: Pose2DResult | NonetoPose3DResult. - Extended
FMPose3DInference.predict(..., return_2d=False)to optionally attach the intermediatePose2DResult. - Updated inference API README to document the new flag and result field.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fmpose3d/inference_api/fmpose3d.py | Adds return_2d plumbing and a new optional pose_2d field on Pose3DResult. |
| fmpose3d/inference_api/README.md | Documents the new return_2d parameter and Pose3DResult.pose_2d field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| progress=progress, | ||
| ) | ||
| if return_2d: | ||
| result_3d.pose_2d = result_2d |
There was a problem hiding this comment.
pose_3d() explicitly takes only the first person when keypoints_2d is 4D (see pose_3d input handling), but here return_2d attaches the full Pose2DResult (potentially containing multiple persons). This makes Pose3DResult.pose_2d ambiguous/inconsistent with the returned 3D poses. Consider either (a) filtering result_2d down to the same person used for lifting before attaching it, or (b) adding/returning an explicit person_index (and documenting that only that person is lifted).
| result_3d.pose_2d = result_2d | |
| # Ensure the attached 2D result matches the person subset used for lifting. | |
| pose2d_for_3d = copy.copy(result_2d) | |
| keypoints_2d = getattr(pose2d_for_3d, "keypoints", None) | |
| if isinstance(keypoints_2d, np.ndarray) and keypoints_2d.ndim == 4: | |
| # pose_3d() uses only the first person when keypoints_2d is 4D; | |
| # mirror that here so pose_2d is consistent with the returned 3D poses. | |
| pose2d_for_3d.keypoints = keypoints_2d[:, :1, ...] | |
| result_3d.pose_2d = pose2d_for_3d |
| if return_2d: | ||
| result_3d.pose_2d = result_2d | ||
| return result_3d |
There was a problem hiding this comment.
New behavior (return_2d=True populating Pose3DResult.pose_2d) isn’t covered by the existing inference API tests. Please add a unit test to assert that pose_2d is None by default and is populated (and consistent with the lifted person) when return_2d=True.
deruyter92
changed the title
return_2d flag for including 2D results in predict() returnreturn_2d flag for including 2D results in predict() return
|
Closing this PR. For separated 2D and 3D steps, use: inference_api = FMPose3DInference(...)
results_2d = inference_api.prepare_2d(...)
results_3d = inference_api.pose_3d(results_2d, ...) |
Motivation:
Currently, if an API-user is interested in the intermediate 2D results, instead of the single
predict()call,they have to run a two stepprepare_2d()+pose_3d(). This PR adds a minor convenience flagreturn_2dto include the intermediate 2D estimation in the returned final results for thepredict()method.hen interested in 2D results before lifting,
Changes:
pose_2dto the output dataclassPose3DResult.predict()populates this field with the 2D pose estimation ifreturn_2d=True