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

[Feature] stim_events_find to retrieve multi-channel stimulus events #976

Merged

Conversation

DerAndereJohannes
Copy link
Contributor

@DerAndereJohannes DerAndereJohannes commented Mar 24, 2024

Description

This PR aims at adding the ability to combine multiple digital input channels into one and extract the stimulus events from the combination of these channels. E.g., If you use 8 digital input channels as a makeshift LPT port (when the main PC does not have an LPT port), the new function converts these channels into an "LPT Channel" and returns all related events.

Proposed Changes

I added the stim_events_find() function that does some preprocessing into the channels to create the stimulus channel and passes this onto the events_find() function to produce events based on the new channel. The conditions list is then updated with the actual value of the event or with a string based on a parsed in value_to_condition dict. Both of these results are then returned.

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • I have read the CONTRIBUTING file.
  • My PR is targeted at the dev branch (and not towards the master branch).
  • I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • I have added the newly added features to News.rst (if applicable)

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.03%. Comparing base (0ea8a2b) to head (016f244).
Report is 25 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #976      +/-   ##
==========================================
+ Coverage   54.60%   56.03%   +1.43%     
==========================================
  Files         304      304              
  Lines       14326    14363      +37     
==========================================
+ Hits         7822     8049     +227     
+ Misses       6504     6314     -190     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DominiqueMakowski
Copy link
Member

Thanks! Could we somehow incorporate this into events_find() to avoid having another function for a special case? I.e., this function would be triggered if we passed multiple channels to events_find?

@DerAndereJohannes
Copy link
Contributor Author

Hi Dominique, thank you for your swift response (and the successful pull on my other request :) )!

My initial plan was to implement it directly into events_find(), but I made this this for 2 reasons:

  1. The additional value_to_condition dict that is specific to this special case
  2. The differing return value

How would you recommend me to integrate this into the events_find() function?

Problem 1 could be an easy fix since I just add the additional input to the end of the arguments list.

Problem 2 I can see multiple solutions:

  1. Have the function return 2 values just for this special case (this feels odd to me)
  2. Have the final dictionary contain the new stimulus channel under a key like "stimulus_channel" (feels probably a bit better to me, but still a bit odd)

Please let me know how I should proceed, or if you have a different solution to this.

Thanks for your support.

@DominiqueMakowski
Copy link
Member

Problem 1: could we incorporate this argument into the current existing event_conditions somehow?
problem 2: option 2 makes sense I think and is convenient from a user experience I'd say

@DerAndereJohannes
Copy link
Contributor Author

Problem 1: could we incorporate this argument into the current existing event_conditions somehow?

Maybe in this case I could just ignore the dictionary input and make it return the numeric value representations for the sake of not complicating the event_conditions input. The user could then transform the conditions list using a dictionary outside of the library (since they have to create the dict with or without this feature, the effort on the user part would be pretty much identical e.g., all they have to do is events["conditions"] = [value[number] for number in events["conditions"]] as a one liner example). What do you think?

problem 2: option 2 makes sense I think and is convenient from a user experience I'd say

Sure, let's do it like that! Do you agree with the "stimulus_channel" name or would you have a better suggestion?

@DominiqueMakowski
Copy link
Member

The user could then transform the conditions list using a dictionary outside of the library

I guess yeah we can document it somewhere by adding an example or so it should be good

"stimulus_channel" name

events_channel?

@DerAndereJohannes
Copy link
Contributor Author

DerAndereJohannes commented Mar 26, 2024

Hi Dominique, with my most recent changes I have integrated the function I proposed directly into the events_find function. I have followed your advice from your previous message and have implemented 2 additional examples highlighting this feature. Please double check how I formatted these in a decent way as this is the first time I have done it in this fashion.

Let me know if there is anything else I can do or if this needs more updating. Thanks!

I think it's a clearer demonstration of a possible usecase
@DominiqueMakowski
Copy link
Member

Great, thanks again. Will merge once it passes

@DerAndereJohannes
Copy link
Contributor Author

Thank you for checking up on my code. Sorry for some of the changes that occurred in the formatting (This was mainly caused by one of the automatic code checks. I will make sure to revert these for future updates.)

The new use case your provide is valid. It would be interesting to see when they are overlapping too, but perhaps this is clear as is. Your example is more to the point.

I have just updated the NEWS file again since I forgot to change it from my previous commit.

@DominiqueMakowski DominiqueMakowski merged commit 8da97e7 into neuropsychology:dev Apr 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants