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

Extracting features gait #4

Merged
merged 36 commits into from
Mar 11, 2024
Merged

Extracting features gait #4

merged 36 commits into from
Mar 11, 2024

Conversation

Erikpostt
Copy link
Contributor

PR contains

  • Updates on imu_preprocessing.py with added functionality for windowing and feature engineering;
  • Directory of .ipynb files for documentation purposes

Waiting with reviewing (and merging) until

  • Currently waiting for merging of PR Refactoring #2 to resolve changes in imu_preprocessing.py;
  • Notebooks are still missing expected output

@Erikpostt Erikpostt self-assigned this Feb 15, 2024
@vedran-kasalica
Copy link
Member

I recommend merging the main branch into this branch after PR #6 receives approval. This step will ensure accurate testing of the changes.

@Erikpostt
Copy link
Contributor Author

@vedran-kasalica what do you recommend here now?

@vedran-kasalica
Copy link
Member

I recommend merging the main branch into this branch after PR #6 receives approval. This step will ensure accurate testing of the changes.

@Erikpostt I would recommend this, but we can also do that together on Thursday.

@Erikpostt
Copy link
Contributor Author

@vedran-kasalica what do you think of the proposed changes?

@vedran-kasalica
Copy link
Member

@vedran-kasalica what do you think of the proposed changes?

The changes look good!
I tested it locally and it all runs well (although I do generate binaries that are different on the byte level). We should probably save the outputs of the Notebooks in a separate folder (and add it to .gitignore so we don't track them) to be able to compare the results to the expected values. When I run it now I override the test data used for comparison.

I will finish reviewing the code tomorrow. We will probably merge the PR and then set up some tests based on your Notebooks and start adding more modules to the toolbox. I have some suggestions (e.g., we probably don't need to store the field meta_filename in our _meta.json files), but I will make a separate PR for that later, so we can distinguish refactoring from this PR.

@vedran-kasalica
Copy link
Member

@Erikpostt it was not clear to me what is the content of rotation_meta file. We don't have an intermediary file specified in the pipeline under that name, right?

@Erikpostt
Copy link
Contributor Author

@Erikpostt it was not clear to me what is the content of rotation_meta file. We don't have an intermediary file specified in the pipeline under that name, right?

You are right Vedran, this is the gyro pre-processed (grey) block at the 2.preprocessed_data level. As to your comment, it would probably be a good idea to be consistent in the naming of the files.

Copy link
Member

@vedran-kasalica vedran-kasalica left a comment

Choose a reason for hiding this comment

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

The changes look good. I tested the Notebooks locally and they run without issues.

Instead of requesting changes to the PR, I opened an issue (see #8). It would be good to address it soon after merging this PR.

I propose to merge the PR and create new PRs to address the follow-up refactoring.

@kretep once you review the PR, feel free to merge it.

@vedran-kasalica vedran-kasalica requested a review from kretep March 6, 2024 11:26
@kretep kretep merged commit 9b10872 into main Mar 11, 2024
1 check passed
@kretep kretep deleted the extracting_features_gait branch March 11, 2024 16:36
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.

3 participants