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.

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

Feature/detect format function #144

Merged

Conversation

cleong110
Copy link
Contributor

@cleong110 cleong110 commented Jan 8, 2025

DRAFT PR:

Adding a function to fix #142. Still TODO:

  • go through the file and replace manual checks with this function.
  • do some more testing to make sure there are no side effects

src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
@cleong110 cleong110 marked this pull request as ready for review January 9, 2025 19:31
@cleong110
Copy link
Contributor Author

cleong110 commented Jan 9, 2025

OK, I've updated throughout generic.py to use the new detect_known_format function. I also added some pytests in, though not comprehensively. But it seems to successfully work with the fake_pose function, detecting the types I expect it to type.

Various functions had various levels of support for the different types. I've gone through and tried to make it more consistent, such that

  • if a type is known not supported it will give an ImportError
  • if a type is unknown it will give a ValueError

src/python/pose_format/utils/generic.py Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
@cleong110
Copy link
Contributor Author

cleong110 commented Jan 10, 2025

OK, ready for the next round, I believe.

  • detect function takes a pose or poseheader now
  • various formatting changes.
  • added some unknown-format poses to the tests, detect_known_format correctly raises an exception.
  • removed the unreachable ValueErrors
  • various type annotations

Copy link
Collaborator

@AmitMY AmitMY left a comment

Choose a reason for hiding this comment

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

Very cool, thank you :)

need to only fix the tests.
instead of from pose_format import Pose i think it needs from pose_format.pose import Pose

@cleong110
Copy link
Contributor Author

cleong110 commented Jan 13, 2025

Taking a crack at fixing the tests. They run on my machine with python 3.10, but crash when run on github on 3.8. Why?

Error:

The error in question:

______________________ ERROR collecting pose_format/utils ______________________
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1014: in _gcd_import
    ???
<frozen importlib._bootstrap>:991: in _find_and_load
    ???
<frozen importlib._bootstrap>:975: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:671: in _load_unlocked
    ???
/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/assertion/rewrite.py:184: in exec_module
    exec(co, module.__dict__)
pose_format/utils/conftest.py:4: in <module>
    from pose_format import Pose
pose_format/__init__.py:1: in <module>
    from pose_format.pose import Pose
pose_format/pose.py:13: in <module>
    from pose_format.utils.generic import pose_normalization_info
pose_format/utils/generic.py:6: in <module>
    from pose_format import Pose
E   ImportError: cannot import name 'Pose' from partially initialized module 'pose_format' (most likely due to a circular import) (/home/runner/work/pose/pose/src/python/pose_format/__init__.py)

Files

src\python\pose_format\__init__.py has

from pose_format.pose import Pose
from pose_format.pose_body import PoseBody
from pose_format.pose_header import PoseHeader

src\python\pose_format\utils\__init__.py is empty

src\python\pose_format\utils\generic_test.py has

from pose_format.pose import Pose

Ah, perhaps conftest.py? It had:

from pose_format import Pose

what if I change everywhere?

  • changing every single instance of pose_format import Pose to pose_format.pose import Pose everywhere in the whole library. Still got errors.

What if I trace the error more closely?

Here's the error output:

pose_format/__init__.py:1: in <module>
    from pose_format.pose import Pose
pose_format/pose.py:13: in <module>
    from pose_format.utils.generic import pose_normalization_info
pose_format/utils/generic.py:6: in <module>
    from pose_format.pose import Pose

That genuinely is a circular import!

OK, so we can't use pose_format.utils.generic in pose_format.pose.pose, because pose_format.utils.generic imports pose_format.pose... how to fix this?

@cleong110
Copy link
Contributor Author

I'm kinda stumped by this pose import circle. Which I think I introduced actually, with the "quick and simple" PR where pose.normalize() would call it by default.

Pose imports utils.generic to get pose_normalization_info
utils imports Pose
^ thus a circle.

Where I'm stuck:

  • Simplest solution might be to remove pose_normalization_info import from Pose
  • but we do want to have that feature, so do we move the implementation into Pose?
  • But then pose_shoulders() needs to move too...

@AmitMY
Copy link
Collaborator

AmitMY commented Jan 13, 2025

Fixed the circular issue at 56e6717

@cleong110
Copy link
Contributor Author

Fixed the circular issue at 56e6717

That'll do it!

@cleong110
Copy link
Contributor Author

cleong110 commented Jan 13, 2025

Pipe annotations causing failures

OK, latest problem is with the pipe type annotations, which prior to 3.10 will throw errors.

https://peps.python.org/pep-0604/

I can either remove all those, or we drop support for 3.8 and 3.9

NotImplementedErrors

Tests for versions 3.10-3.12 are currently expected to fail with NotImplementedErrors. We can either leave them failing as a reminder to implement, or just add a with pytest.raises to catch those.

@AmitMY
Copy link
Collaborator

AmitMY commented Jan 13, 2025

I susggest we use Union instead of the pipe, to support older versions

@cleong110
Copy link
Contributor Author

While trying to get tests to pass, I discovered two problems with correct_wrist:

  1. It was modifying the input Pose's data despite returning a Pose object. Added a deepcopy operation to prevent this.
  2. It was creating stacked_conf with shape 3 points (XYZ), even if the format called for 2 (XY). Changed out the hardcoded "3" to check the shape of the data, thus:
point_coordinate_count = wrist.shape[-1]
    stacked_conf = np.stack([wrist_conf] * point_coordinate_count, axis=-1) 

@cleong110
Copy link
Contributor Author

All tests now green on my machine!

src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
src/python/pose_format/utils/generic.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@AmitMY AmitMY left a comment

Choose a reason for hiding this comment

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

great! thanks for the hard work :)

@AmitMY AmitMY merged commit f319e3e into sign-language-processing:master Jan 14, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

generic utils raises error if components are not in expected order: add detect_format function?
2 participants