-
Notifications
You must be signed in to change notification settings - Fork 0
Clean up toolbox #20
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
base: main
Are you sure you want to change the base?
Clean up toolbox #20
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
===========================================
- Coverage 100.00% 91.58% -8.42%
===========================================
Files 1 5 +4
Lines 2 321 +319
===========================================
+ Hits 2 294 +292
- Misses 0 27 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
behinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some further comments:
- Any reason to have the ride methods in a folder ride?
- Any reason that the ride methods have a
ride_in the filenames? - The docs currently dont show any plots
- you seem to have still quite some code doubling in
ride_classic_algorithmandride_unfold_algorithm. I mostly checked the "prepare_data" phase, which reads near 1:1. I generally would prefer to have one "master"-loop?+´ - I think similarities between both approaches would appear starkly, once the code is refactored into smaller functions. Right now it is a very large near-monolithic function
I didnt look much more into the functions, because if you decide to put both methods together, I wouldnt need to review two separate files :)
| r_range = [0, 0.8], | ||
| c_range = [-0.4, 0.4], | ||
| c_estimation_range = [-0.1, 0.9], | ||
| formulas = [@formula(0 ~ 1), @formula(0 ~ 1), @formula(0 ~ 1 + reaction_time)] # formulas used for S, R, and C component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to do:
formulas = ['S'=>@formula(0~1),'C'=>@formula(0~1),'R'=>@formula(0~1)], e.g. as a list of pairs? that would generalize to more events more readily and doesnt have the issue of order. You the only have to splice in the firbasis in the fit command down below
Related issues
Closes #
Checklist