-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: add text stimulus class #676
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #676 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 55 57 +2
Lines 2530 2559 +29
Branches 644 647 +3
=========================================
+ Hits 2530 2559 +29 ☔ View full report in Codecov by Sentry. |
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.
Thank you so much for working on this task! I think the current startout supposes too big of a scope of this PR. We should keep the scope very limited and work iteratively on adding new functionalities in distinct PRs. The TextStimulus
class will be experimental for a while, but I think this strategy is better than having too big of a scope in this big project.
This PR should focus on the basics first and only, returning a TextStimulus
class which has an aoi
attribute when calling pymovements.stimulus.text.from_file()
.
We'll discuss next friday if it should be The class |
5ae4a6e
to
f09156b
Compare
07f9229
to
b80de51
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
Thanks a lot, that's a good direction the PR is taking!
There is some more work needed around the edges but we're getting to it soon!
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.
Great! Just two minor things and then there's the question about if we should clone a input dataframe.
On a more general note, I would like to discuss tomorrow if we want the naming to be
Stimulus.aois
- or
Stimulus.areas
I orignally proposed aois
, but somehow I think areas
would be a better fit.
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.
Excellent! There are some tiny bits in the __init__.py
and the docstrings need parameter specifications but that's about it.
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.
Excellent! Thank you so much for this work!
No description provided.