Test main pipeline regressions - #290
Conversation
9bee73e to
86313a5
Compare
carmichaelong
left a comment
carmichaelong
left a comment
There was a problem hiding this comment.
Thanks, @csherry04! I left a bunch of notes throughout. Overall, biggest things to clean up are what's getting copied to make sure tests may not "accidentally" pass and would better replicate the real structure of what inputs look like at run time.
| pd.testing.assert_frame_equal( | ||
| output_trc_df, ref_trc_df, check_exact=False, atol=atol | ||
| ) | ||
| assert output_trc_df.isna().sum().sum() == ref_trc_df.isna().sum().sum() |
There was a problem hiding this comment.
is this assert necessary given assert_frame_equal above?
There was a problem hiding this comment.
I can't seem to find it in their documentation, but some stack overflow posts say that nan's should be treated correctly with assert_frame_equal. If that's the case, this assert seems like a weaker condition (just counting)
| dataDir=dataDir, | ||
| genericFolderNames=True, | ||
| poseDetector='openpose', | ||
| resolutionPoseDetection='1x736', |
There was a problem hiding this comment.
Consider adding syncVer=1.0 here, so the test doesn't change even if default sync version changes.
| trialID = trialName | ||
| dataDir = tmp_path | ||
| sessionDir = os.path.join(dataDir, 'Data', sessionName) | ||
| shutil.copytree( |
There was a problem hiding this comment.
It could be better to only copy the necessary files, rather than all the files.
There was a problem hiding this comment.
Might also be helpful to add some short comments for maintenance to explain the high-level strategy and location of copying these files. It's a little hard to trace (see comments below)
There was a problem hiding this comment.
Or if the copies are simplified, might not need a comment if it's pretty straightforward like in some other cases here
There was a problem hiding this comment.
have added helper to copy in just the necessary files, hopefully makes it more scoped and understandable
| ) | ||
|
|
||
| for camName in ['Cam0', 'Cam1']: | ||
| production_pickle = os.path.join( |
There was a problem hiding this comment.
Looks like a whole directory is copied, but then extra copies are needed for a specific file to a different specific folder (local_pickle_dir)? I might be missing something, but seems like copies could be streamlined
There was a problem hiding this comment.
this was due to the fact that the path expected was different to what was in the original directory so was first copying the full thing then correcting the path to the pickle, should be fixed with the new helper
| 'PreAugmentation', | ||
| f'{trialName}.trc', | ||
| ) | ||
| assert os.path.exists(output_pre_augmentation_trc) |
There was a problem hiding this comment.
Why assert just pre-augmented markers (rather than both along with post-augmented, or neither)? Can main() exit without throwing an error (and thus these asserts would be needed)?
There was a problem hiding this comment.
agree was unnecessary to do any intermediate check, removed
| trialID = trialName | ||
| dataDir = tmp_path | ||
| sessionDir = os.path.join(dataDir, 'Data', sessionName) | ||
| shutil.copytree(LAB_5CAM_DIR, sessionDir) |
There was a problem hiding this comment.
Same comments here, as with the above neutral scaling test
| f'{trialName}.mot', | ||
| ) | ||
| # excludes the setup motion | ||
| compare_mot_files(output_mot, ref_mot, 2, 9) |
There was a problem hiding this comment.
Suggest using 2.0 and 9.0 just to make it clearer these are floats (even though it's less of an issue with Python 3+)
| 'Model', | ||
| 'LaiUhlrich2022_scaled.osim', | ||
| ) | ||
| assert os.path.exists(scaled_model) |
There was a problem hiding this comment.
One issue with copying the whole folder tree is that the scaled_model here looks to be part of the copy, so this assert might never fail (or actually silently let errors through)
2 participants
With this main test file will have added calibration, neutral, 5 cam calibration, and 5 cam motion tests. Some helpers added for readability as well