Added sync to metadata yaml#275
Conversation
dev -> main
main: new intrinsics (iphone 17, ipad m5)
carmichaelong
left a comment
carmichaelong
left a comment
There was a problem hiding this comment.
Thanks @AlbertoCasasOrtiz, left a minor comment and opened a discussion. Overall looks good though.
| ValueError: If the sync version is not supported. | ||
| """ | ||
| if syncVer == '1.0': | ||
| print("Using synchronization algorithm v1.0") |
There was a problem hiding this comment.
There's a few of these print lines in utilsSync.py that appear to be debugging checks (since we do print out this info to users already). I think these print functions could either be removed, or use logging.debug() if you think they could be helpful later
There was a problem hiding this comment.
I'm leaning towards using logging.debug() just in case. We can always remove it later.
| try: | ||
| session_desc["sync_ver"] = session['meta']['settings']['sync_ver'] | ||
| except: | ||
| session_desc["sync_ver"] = '1.1' |
There was a problem hiding this comment.
What do we think the correct value to fill here is? I think there are a few interpretations:
1.0: this makes it consistent for sessions that did not have async_verbefore, so thisexceptbranch is answering what to do if nosync_verexists.1.1: we default to the latest sync version, since we think it should work the bestDEFAULT_SYNC_VERfromdefaults.py: we keep all default values in line across the repository
Perhaps one way to do this (separating the defaults in this processing repo vs a user in the web app):
- use
DEFAULT_SYNC_VERin this repo - in
opencap-viewer, make sure to send1.0for a session that has nosync_ver
Definitely open to thoughts here, just tried to lay out a few options.
There was a problem hiding this comment.
I guess if 1.1 fixes trials that were causing the error, maybe defaulting to 1.1 would be better so people's trials are fixed upon reprocessing.
Using DEFAULT_SYNC_VER sounds like the better option to me. DEFAULT_SYNC_VER is currently assigned to 1.0, but we could change it to 1.1
There was a problem hiding this comment.
I agree DEFAULT_SYNC_VER and changing default to 1.1 seems like the best option here
|
A couple more things:
|
Done! |
| 'augmentermodel': 'v0.2', | ||
| 'filterfrequency': '12', | ||
| 'scalingsetup': 'upright_standing_pose', | ||
| 'sync_ver': '1.1' |
There was a problem hiding this comment.
if default value is changed to 1.1 maybe make one of these 1.0 to test whether it correctly captures value returned by api when it differs from default
| 'checkerboard': { | ||
| 'square_size': 25.0, | ||
| 'cols': 7, | ||
| 'rows': 5, | ||
| 'placement': 'ground' | ||
| } |
There was a problem hiding this comment.
I could be wrong but I believe the test_get_metadata_with_calibration_reference won't actually reference the second session because the "checkerboard" meta data is present in this first session. So could remove the checkerboard data here?
3 participants
This should be merged with the corresponding PRs: