Issues regarding PhysicalActivityMonitoring algorithm

neurogeriatricskiel / KielMAT

Python based toolbox for processing motion data

https://neurogeriatricskiel.github.io/KielMAT/

Other

6 stars 1 forks source link

Issues regarding PhysicalActivityMonitoring algorithm #73

Closed masoudabedinifar closed 6 months ago

masoudabedinifar commented 7 months ago

Dear all,

The issues that need to be addressed regarding the PhysicalActivityMonitoring algorithm before submitting the JOSS paper could be listed as follows:

Tracked point for physical activity monitoring

[x] "Why is this hard-coded? What if I would want to do the analysis for the right side? What if I choose to name my tracked point "leftWrist" and "rightWrist" or "leftHand" and "rightHand"?" as mentioned here Tracked point for physical activity monitoring

Accelerometer unit conversion for physical activity monitoring

[x] "Why is this hard-coded in the algorithm? What happens if the accelerometer data are already in g? Should this not be checked using the channels information?" as mentioned here Accelerometer unit conversion for physical activity monitoring

Change data columns in a general way to apply to any kind of dataset

[x] Make this calculation general: data["en"] = np.linalg.norm(data[["LARM_ACCEL_x", "LARM_ACCEL_y", "LARM_ACCEL_z"]], axis=1)

masoudabedinifar commented 6 months ago

I have addressed all issues in commit: 5c8617c35567cd612606cff67332c3b41024a9d9. Could you please review the changes? If you approve them, then please proceed to close the issues.

JuliusWelzel commented 6 months ago

Can you open a PR and ask for a review or point us to the lines we should be reviewing?

masoudabedinifar commented 6 months ago

Can you open a PR and ask for a review or point us to the lines we should be reviewing?

I merged this directly into the main branch with the commit: 5c8617c35567cd612606cff67332c3b41024a9d9. I'm uncertain whether it's possible to create a pull request after pushing the commit. Thank you!"

JuliusWelzel commented 6 months ago

I have reviewed the code. Looks good, only mean calculation is optimized in https://github.com/neurogeriatricskiel/NGMT/commit/4b609f031bf496dcc113da624cd8cd56963f90cd

JuliusWelzel commented 6 months ago

Another thing is, that the index data currently has to be a in datetime which is fine but also has to be named 'timestamp'. You can check this in this example: https://github.com/neurogeriatricskiel/NGMT/blob/main/examples/03a_tutorial_physical_activity_monitoring_with_load.ipynb

Please fix this in the https://github.com/neurogeriatricskiel/NGMT/blob/29586fffbee07ce86345ef3737453536e6bfb21c/ngmt/utils/preprocessing.py#L1004 function

masoudabedinifar commented 6 months ago

Hi @JuliusWelzel thanks! I fixed and updated the function and all other files accordingly in 0eb233f2b264ebd7735ebaf25a8686c79e342925 and I am going to close the issue.

JuliusWelzel commented 6 months ago

@masoudabedinifar Thanks for fixing. However, index_name is a to generic term. Could you change it to be more meaningful, such as time_column_name?

masoudabedinifar commented 6 months ago

@JuliusWelzel name was changed to time_column_name with commit: f8ef5c41b0310453ad674292b3fc2ea19a8fef19