dougollerenshaw
commented
3 years ago Thanks @yavorska-iryna. I definitely understand the potential for confusion and I think it comes down to poor variable name choices on my part. But I'd argue that calling the variable t_before necessitates that this is the time before each event as a positive number. I.e., saying t_before = 1 implies that the time range should start 1 second before the event.
Alternately, if we were to change it, a user would pass t_before = -1 to maintain the current behavior. I'd argue that this would be more confusing because the time range would not be starting -1 seconds before the event, as the variable name implies.
So, how about this as an alternate solution?:
- we leave
t_beforeandt_afteras is, but we are clearer about their behavior in the docstring. - we add two new variables:
t_startandt_end, that would act exactly as you described above. - we add an assertion statement that ensures that users cannot pass both
t_beforeandt_start, or botht_afterandt_end.
In addition to providing more clearly labeled variable names, this solution would have the additional benefit of not affecting existing behavior.
Currently,
t_beforeis passed as integer or float with a positive sign, then changed to a negative number, which is hardcoded in the function. This poses an issue because in order to be able to pass at_beforevalue that is post event onset, it needs to be negative. For instance, if you are aligning traces 3 seconds prior to an event and 3 seconds post event, you would passt_before=3andt_after=3. But if you are aligning traces 1 second post event to 3 seconds post event, you would need to passt_before=-1andt_after=3, which is confusing and unintuitive. This needs to be changed thatt_beforeis passed with a desired sign, making itt_before=-3andt_after=3in the first case andt_before=1andt_after=3in the second case.