Sync 1.1 and testing

[carmichaelong]

The main purpose of this PR is to improve detection and syncing with an arm raise (hand punch). It also adds testing and adds a new utilsSync.py to separate some code. On a high level, this PR:

• Adds new methods for detectHandPunchAllVideos() and syncHandPunch() to handle the old (v1) and (v2) versions
• Adds a new flag for synchronization version (syncVer), using 1.0 for the previous methods and 1.1 to use the new hand punch methods (gait and general kept the same)
• Regression/integration tests, and unit tests related to syncing. Added a submodule to the new opencap-test-data repo so that users don't have to download larger files if they won't run tests.
• Splits sync-specific code into utilsSync.py (from utilsChecker.py). This unfortunately makes the file look like a very large diff, but changes were limited to the following functions:
  • synchronizeVideoKeypoints()
  • detectHandPunchAllVideos()
  • detectHandPunchAllVideos_v1()
  • detectHandPunchAllVideos_v2()
  • syncHandPunch()
  • syncHandPunch_v1()
  • syncHandPunch_v2()
• Adds a new file defaults.py for easier handling of default values

In particular, the following issues with the hand punch detection and synchronization were addressed:

• Detection
  • Multiple punches could ignore detection completely (specifically, this PR allows multiple punches as long as the 2nd highest punch is not more than 70% of the highest punch to avoid ambiguous syncing)
  • Low confidence data could cause an anomalous extra punch (which would then prevent the trial from using the clean hand punch)
  • The arm started above the shoulder at the start of a trial, which would not be detected
  • If there was both a right and left hand punch, the right hand punch would be used every time (even if the left hand punch was higher)
  • If the cameras disagree about which hand was used, the behavior was undefined
  • Very short duration punches (not actually a punch) from noisy data were not filtered out
  • Arm punch might happen at a time when the ankle markers have low confidence
• Syncing
  • Low confidence data could be used when syncing (even with a successful detection of a hand punch), causing an incorrect lag shift
  • Gaussian blur on the confidence trace could shift a true peak toward zero (e.g., the lag found was smaller than the true lag as it shifts away from the true correlation peak)
  • Correlation was computed on the whole trial (or more specifically, where the ankle markers had high confidence), rather than just around the clean hand punch

I'm assigning @AlbertoCasasOrtiz for the review, though it's become such a large PR a second review might be warranted (@mattpetrucci would be good for this if we think it's needed).

@suhlrich @antoinefalisse Just tagging you here as a heads up, but no review is necessary.

[mattpetrucci]

Generally, looks good. Just need to have the tests pass on a Windows machine.

[mattpetrucci]

Tests failed on my Windows machine. Here is the output:

PASSED tests/test_api.py::test_get
PASSED tests/test_api.py::test_put
PASSED tests/test_api.py::test_success_after_retries
PASSED tests/test_sync.py::test_synchronize_videos[squats-Using general sync function.-1.0]
PASSED tests/test_sync.py::test_synchronize_videos[squats-Using general sync function.-1.1]
PASSED tests/test_sync.py::test_synchronize_videos[walk-Using gait sync function.-1.0]
PASSED tests/test_sync.py::test_synchronize_videos[walk-Using gait sync function.-1.1]
PASSED tests/test_sync.py::test_synchronize_videos[squats-with-arm-raise-Using handPunch sync function.-1.0]
PASSED tests/test_sync.py::test_synchronize_videos[squats-with-arm-raise-Using handPunch sync function.-1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_clean_right_or_left[1.0-r-punch_range0-None]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_clean_right_or_left[1.1-r-punch_range1-ref_handPunchRange1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_clean_right_or_left[1.0-l-punch_range2-None]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_clean_right_or_left[1.1-l-punch_range3-ref_handPunchRange3]
PASSED tests/test_sync.py::TestDetectHandPunch::test_no_punch[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_no_punch[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_both_hands_same_time[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_both_hands_same_time[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_both_hands_different_times[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_both_hands_different_times[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_multiple_punches_different_height[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_multiple_punches_different_height[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_multiple_punches_similar_height[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_multiple_punches_similar_height[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_low_confidence[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_low_confidence[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_too_short[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_too_short[1.1]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_too_long[1.0]
PASSED tests/test_sync.py::TestDetectHandPunch::test_punch_too_long[1.1]
PASSED tests/test_sync.py::TestSyncHandPunch::test_syncver_lag_between_cameras[1.0-5-5]
PASSED tests/test_sync.py::TestSyncHandPunch::test_syncver_lag_between_cameras[1.1-5-5]
PASSED tests/test_sync.py::TestSyncHandPunch::test_syncver_lag_between_cameras[1.0-120-103]
PASSED tests/test_sync.py::TestSyncHandPunch::test_syncver_lag_between_cameras[1.1-120-120]
PASSED tests/test_sync.py::TestSyncHandPunch::test_sync_hand_punch_v2_stability[position-None]
PASSED tests/test_sync.py::TestSyncHandPunch::test_sync_hand_punch_v2_stability[position-6.0]
PASSED tests/test_sync.py::TestSyncHandPunch::test_sync_hand_punch_v2_stability[velocity-None]
PASSED tests/test_sync.py::TestSyncHandPunch::test_sync_hand_punch_v2_stability[velocity-6.0]
FAILED tests/test_main.py::test_main[squats-with-arm-raise-5.0-10.0-1.0] - AssertionError: DataFrame.iloc[:, 3] (column name=“3”) are different
FAILED tests/test_main.py::test_main[squats-with-arm-raise-5.0-10.0-1.1] - AssertionError: DataFrame.iloc[:, 3] (column name=“3") are different
FAILED tests/test_main.py::test_main[squats-3.0-8.0-1.0] - AssertionError: DataFrame.iloc[:, 3] (column name=“3”) are different
FAILED tests/test_main.py::test_main[squats-3.0-8.0-1.1] - AssertionError: DataFrame.iloc[:, 3] (column name=“3") are different
FAILED tests/test_main.py::test_main[walk-1.0-5.0-1.0] - AssertionError: DataFrame.iloc[:, 3] (column name=“3”) are different
FAILED tests/test_main.py::test_main[walk-1.0-5.0-1.1] - AssertionError: DataFrame.iloc[:, 3] (column name=“3") are different
========================================================================================================================================================================================== 6 failed, 37 passed, 19 warnings in 77.19s (0:01:17) ==========================================================================================================================================================================================

[carmichaelong]

Updated the test to be looser (markers post-augmenter should now checked within 1mm per frame) and tested on a Windows machine that this passes on there now. Probably worth checking locally on your machine too.

[mattpetrucci]

All tests pass now on my Windows machine.

[AlbertoCasasOrtiz]

From what I've seen everything seems alright (although one test didn't pass, probably due to my env). Only concerns are related to the maintainability of the code.

[AlbertoCasasOrtiz]

Failed for me in Windows, the output gives:

(opencap) PS C:\Workspaces\opencap\opencap-core> 
================================================================================= test session starts =================================================================================
platform win32 -- Python 3.9.23, pytest-8.4.2, pluggy-1.6.0
rootdir: C:\Workspaces\opencap\opencap-core
collected 1 item                                                                                                                                                                       
run-last-failure: rerun previous 1 failure (skipped 2 files)

tests\test_api.py F                                                                                                                                                              [100%]

====================================================================================== FAILURES =======================================================================================
_____________________________________________________________________________ test_success_after_retries ______________________________________________________________________________

mock_response = <MagicMock name='_get_conn' id='2936532450464'>

    @patch("urllib3.connectionpool.HTTPConnectionPool._get_conn")
    def test_success_after_retries(mock_response):
        mock_response.return_value.getresponse.side_effect = [
            Mock(status=500, msg=HTTPMessage()),
            Mock(status=502, msg=HTTPMessage()),
            Mock(status=200, msg=HTTPMessage()),
            Mock(status=429, msg=HTTPMessage()),
        ]

>       response = makeRequestWithRetry('GET',
                                        'https://test.com/',
                                        retries=5,
                                        backoff_factor=0.1)

tests\test_api.py:66:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
utils.py:1765: in makeRequestWithRetry
    response = session.request(method,
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\requests\sessions.py:589: in request
    resp = self.send(prep, **send_kwargs)
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\requests\sessions.py:703: in send
    r = adapter.send(request, **kwargs)
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\requests\adapters.py:644: in send
    resp = conn.urlopen(
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\urllib3\connectionpool.py:946: in urlopen
    retries.sleep(response)
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\urllib3\util\retry.py:355: in sleep
    slept = self.sleep_for_retry(response)
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\urllib3\util\retry.py:332: in sleep_for_retry
    retry_after = self.get_retry_after(response)
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\urllib3\util\retry.py:329: in get_retry_after
    return self.parse_retry_after(retry_after)
C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\urllib3\util\retry.py:307: in parse_retry_after
    if re.match(r"^\s*[0-9]+\s*$", retry_after):
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

pattern = '^\\s*[0-9]+\\s*$', string = <Mock name='mock.headers.get()' id='2936533140720'>, flags = 0

    def match(pattern, string, flags=0):
        """Try to apply the pattern at the start of the string, returning
        a Match object, or None if no match was found."""
>       return _compile(pattern, flags).match(string)
E       TypeError: expected string or bytes-like object

C:\Users\NMBL\anaconda3\envs\opencap\lib\re.py:191: TypeError
---------------------------------------------------------------------------------- Captured log call ---------------------------------------------------------------------------------- 
DEBUG    urllib3.connectionpool:connectionpool.py:549 [https://test.com:443](https://test.com/) "GET / <MagicMock name='_get_conn()._http_vsn_str' id='2936533074656'>" 500 <Mock name='mock.length_remaining' id='2936533090704'>
DEBUG    urllib3.util.retry:retry.py:517 Incremented Retry for (url='/'): Retry(total=4, connect=None, read=None, redirect=None, status=None)
================================================================================== warnings summary =================================================================================== 
..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:10
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:10: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    _nlv = LooseVersion(_np_version)

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:11
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:11: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    _np_version_under1p16 = _nlv < LooseVersion("1.16")

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:12
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:12: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    _np_version_under1p17 = _nlv < LooseVersion("1.17")

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:13
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:13: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    _np_version_under1p18 = _nlv < LooseVersion("1.18")

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:14
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:14: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    _np_version_under1p19 = _nlv < LooseVersion("1.19")

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:15
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\__init__.py:15: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    _np_version_under1p20 = _nlv < LooseVersion("1.20")

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\setuptools\_distutils\version.py:336
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\setuptools\_distutils\version.py:336: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    other = LooseVersion(other)

..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\function.py:125
..\..\..\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\function.py:125
  C:\Users\NMBL\anaconda3\envs\opencap\lib\site-packages\pandas\compat\numpy\function.py:125: DeprecationWarning: distutils Version classes are deprecated. Use packaging.version instead.
    if LooseVersion(_np_version) >= LooseVersion("1.17.0"):

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================================================== short test summary info =============================================================================== 
FAILED tests/test_api.py::test_success_after_retries - TypeError: expected string or bytes-like object
============================================================================ 1 failed, 9 warnings in 3.04s ============================================================================ 
(opencap) PS C:\Workspaces\opencap\opencap-core>

[carmichaelong]

I could not replicate this on a local machine on Windows, Mac or Linux. The patch response is indeed quite far in, so it's tricky (I tried a little bit to simplify it, but the hook is very deep for the retry). Let me know if you're OK with it as is, or one option would be to remove the test (it's mostly an off-the-shelf use case of the requests library).

[AlbertoCasasOrtiz]

If that works for you and Matt I think it should be OK. I had a hard time setting out my environment, so it was probably something related to my local setting.

[AlbertoCasasOrtiz]

I wonder if this file can be further broken down for clarity and a clearer pipeline. Maybe into different types of tasks (detection, synchronization of hand punch, helpers...).

It could be useful to, in the future, apply object oriented programming in this repo, so we can better organize it.

[carmichaelong]

I agree with this in general. The goal of the PR was to improve sync, with refactoring into a separate file as a separate step to start breaking it down. I was reluctant to also break it down to further to make the PR even harder to read, but open to hearing if there are any other steps you'd like to see in this PR specifically, and what steps should be in a different issue.

[AlbertoCasasOrtiz]

I think we should close this PR and try to do this in the future. Maybe we should create an issue to brainstorm ideas.

[carmichaelong]

Thanks @AlbertoCasasOrtiz and @mattpetrucci!