Change absolute time reference to first go cue

[ZhixiaoSu]

The line that generates reference time point when calculating absolute time was modified to the start of go cue of the first trial. This is related to #49. The branch is tested in codeocean capsule.

[alexpiet]

@ZhixiaoSu @rachelstephlee There are actually a few places in the code where we used the start of the first trial instead of the first go cue.

I'm also taking this opportunity to add checks for time alignment in general

[rachelstephlee]

yes, the changes came from here:

c324d07#diff-ccea6bb49cf8b0e9e7ac5e68edea5d05160f676cfcb2918d0944837992a04122R328

[rachelstephlee]

again, same thing here-- yet another note to "goCue_start_time" that is buried in here

[rachelstephlee]

Let's talk more monday, but my main concern with the code is that there's still separate references to 'goCue_start_time' instead of a uniform reference to t0, which would be cleaner (ideally, i would wish the alignment choic would be for the entire package, but not sure if that could work).

my other issue is that in the alignment package, people send in event_times for event_triggered_response, but if we allow people to set an nwb to have align_time = True in some function calls in nwb_utils but false in others for the same NWB, people will be aligning to the wrong event_times altogether when they call alignment.

This might be a known user error... but if we're building more complicated functions that rely on nwb's having df_events, df_fib etc all set, this would be particularly confusing/messy.

[ZhixiaoSu]

I think it is better to skip the absolute time alignment to t0 (first go cue). 1. When harp clock is shared across multiple recording domains and computers (e.g. ephys), it can be tricky to make sure all timestamps stored at different locations aligned in a consistent way. And any mis-alignment can cause major issue. 2. It does not add much value by having all session aligned to the first go cue: we probably won't plot/analyze them all together with this alignment.

[rachelstephlee]

@alexpiet , let us know if the fixes ew discussed in person will be implemented, me + sue will do another code review, and we should be set.

afaik, i think i just wanted:

1. the string "goCue_start_time_in_session" to be made into a global variable since it's used across 4 function calls
2. use "raw" instead of "absolute" (so for df_fib, df_events, it should be timestamps_raw, timestamps rather than timestamps_absolute, timestamps)
3. Have the 't0' column in df_trials to be renamed 'goCue_time_raw' (to match time<raw/in_session/in_trial>)

• if you want to wait until we rename all the goCue_starttime to goCue, you can leave it as is as 'goCue_starttime'
• or if you don't mind putting in the change, just rename all of goCue_starttime to goCue (for goCue_time_in_session, for goCue_time_in_trial etc).

[alexpiet]

@rachelstephlee @ZhixiaoSu This is ready for review again

[alexpiet]

@rachelstephlee @ZhixiaoSu Please review when you have a chance

[rachelstephlee]

Looks good-- some nitpick changes (that you can ignore or punt to a new issue).

Main changes before approval would be:

1. line 362 has a bug
2. what is 'data' in df_events ? didn't understand what it meant, not sure if it's a bug

Nice to have, but can fix later:

1. ideally, warnings in df_trial should provide the trial Change absolute time reference to first go cue #50
2. warning for adjusting time should not use t0 but a different message instead

any of the other discussion points, please feel free to answer/punt to a new issue/ignore if you disagree.

[alexpiet]

@rachelstephlee resolved your comments. ready for review again

[rachelstephlee]

looks great. thank you for putting this together and going thru comments again and again.