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

Calculate height for marker augmenatation #152

Merged
merged 15 commits into from
Jan 6, 2025

Conversation

hunminkim98
Copy link
Contributor

@hunminkim98 hunminkim98 commented Dec 8, 2024

Hello, Dr. David,

Here are some modifications related to marker augmentation. All of these changes were implemented using your existing excellent functions!

  • I added a few lines to utilize the compute_height function in markerAugmentation.py.
  • I changed the default value of close_to_zero_speed from 50 to 0.2. I believe the value 50 is specific to Sports2D.
  • I added some code to handle the case where the mid-hip marker is missing. I also attempted to simplify the implementation as per your suggestion, but this resulted in an error because the structure stores X, Y, and Z coordinates consecutively. Processing them all at once causes issues with the sorting and indexing of the DataFrame.

And I have commented out the part in the current config that loads subject_height. If you prefer complete automation without user input, I will remove it. On the other hand, if you would like automatic height calculation to occur only when set to 'auto,' I can make some adjustments accordingly.

Additionally, parameters such as close_to_zero_speed_m, large_hip_knee_angle, and trimmed_extrema_percent can be loaded from the config to ensure that the default values are not the only ones being used.

Thank you!

@hunminkim98
Copy link
Contributor Author

Oh no, it seems there's an issue with pytest. This error occurred when all frames were filtered. I'll investigate why all the values were excluded and make the necessary corrections.
Could not compute height from Demo_MultiPerson_P3_0-100_filt_butterworth.trc. Error: At least one of the following markers is missing for computing the height of the person: RHeel, RAnkle, RKnee, RHip, RShoulder, LHeel, LAnkle, LKnee, LHip, LShoulder.

Copy link
Collaborator

@davidpagnon davidpagnon left a comment

Choose a reason for hiding this comment

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

Hi,

  • Great stuff!
  • It does not matter what the default value is, but it is probably more important to call the function with the right values. And I'd rather have the exact same functions on both toolboxes. So L133, you should specify the right values instead of taking the default ones. Instead of height = compute_height(Q_coords_filtered, filtered_markers),
    it should be
    height = compute_height(Q_coords_filtered, filtered_markers, fastest_frames_to_remove_percent=fastest_frames_to_remove_percent, close_to_zero_speed=close_to_zero_speed, large_hip_knee_angles=large_hip_knee_angles, trimmed_extrema_percent=trimmed_extrema_percent)
  • Okay! I suggested something different in the last commit for best_coords_for_measurements, could you check if it works?
    I am not sure lines 115 to 131 are needed in markerAugmentation, can you try without them?

It would be nice to have the option of setting the height automatically or not indeed! And I think that heights and weights should now be moved to the [markerAugmentation] section in Config.toml.

When everything works, it would be good to remove all the prints!

@davidpagnon
Copy link
Collaborator

Sorry I commented too fast on my last comment!

@davidpagnon
Copy link
Collaborator

davidpagnon commented Dec 10, 2024

It seems like it passes the tests now, congrats! 🎉 Does it seem like it works as expected on your end?

A few minor remarks:

  • Could you make participant_height = 'auto' the default behavior?
    Also, don't forget to add it to the Config files in the subfolders for Demo_batch :)
  • Do you think it is possible to let fastest_frames_to_remove_percent and other measurement parameters in the [kinematics] section instead of [project]?
  • I feel like read_trc, compute_height, and best_coords_for_measurements should be moved to common.py, what do you think?
  • Instead of print(f"Subject height for {os.path.basename(trc_file)}: {height} m"), can you do logging.info(f"Subject height automatically calculated for {os.path.basename(trc_file)}: {height} m")
  • L361 to 363 in kinematics.py, you did it slightly differently than me but I think it should be equivalent so no problem

@hunminkim98
Copy link
Contributor Author

hunminkim98 commented Dec 10, 2024

Hello, Dr. David,

I apologize for the delayed response. I wanted to reply after completing the revisions, but some unexpected tasks came up, and I could only respond now.
Thank you always for your detailed and positive feedback!
The feedback from your previous review has been successfully incorporated, and I also made a few minor additional adjustments to ensure everything works as expected.

From my tests, everything functioned as we discussed. Below are my responses to your latest review and suggestions:

  • Yes, absolutely! Setting 'auto' as the default makes perfect sense to me.
  • I had moved fastest_frames_to_remove_percent and other parameters from the [kinematics] section to the [markerAugmentation] section earlier. Are you suggesting moving them back to the [kinematics] section?
  • I completely agree! Moving them to common.py for broader usage sounds like a great idea.
  • Absolutely, thank you for the excellent suggestion. I will update it accordingly!
  • Yes, it achieves the same purpose perfectly. In my tests, there was an issue with generating six columns, so I fixed it in the current version.

@davidpagnon
Copy link
Collaborator

No rush at all!

Sorry, the parameters are good where they are, I misread the report:
image

And okay for the rest :)

@hunminkim98
Copy link
Contributor Author

I understand! Thank you for reviewing it again. I'll apply all the other suggestions!

@hunminkim98
Copy link
Contributor Author

hunminkim98 commented Dec 10, 2024

I was wondering, do you think the 'mean_angles' function could also be moved into common.py?

Edit: Oops, I'm sorry! The function depends on angle_dict, so moving the 'mean_angles' function might make things messy.

@hunminkim98
Copy link
Contributor Author

Hello, now everything works fine as I expected.

@davidpagnon davidpagnon marked this pull request as draft January 6, 2025 23:44
@davidpagnon davidpagnon self-assigned this Jan 6, 2025
@davidpagnon davidpagnon merged commit 8e4ab2a into perfanalytics:main Jan 6, 2025
12 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.

2 participants