Skip to content

Add epoch information for TTL channels #1791

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.

Sign up for GitHub

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

Jump to bottom
Merged
t-b merged 24 commits into from
Jul 13, 2023
Merged

Add epoch information for TTL channels #1791

t-b merged 24 commits into from
Jul 13, 2023

Conversation

@MichaelHuth
Copy link
Collaborator

@MichaelHuth MichaelHuth commented Jun 21, 2023

close #855

@MichaelHuth MichaelHuth self-assigned this Jun 21, 2023
@MichaelHuth MichaelHuth mentioned this pull request Jun 21, 2023
Merged
@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch 4 times, most recently from a96fb77 to d1f4bef Compare June 28, 2023 02:44
@t-b
Copy link
Collaborator

t-b commented Jun 28, 2023

Its way too late here for a proper review but I think that in 5149f3d the indexing of statusTTLFiltered is wrong. It should be by active channel aka i. See how DC_ITC_MakeTTLWave does it in main.
Also the commit changing behaviour should also have the changed tests.
And every commit which adds/changes labnotebook entries (including the meaning) should raise the labnotebook version. Otherwise I don't find all the places where the LBN was changed with tools/create-changelog.sh.
2a04ea5 needs a "introduced in XXX" part of the commit message and an explanation why this was not found earlier including the consequences of the bug. And a tests which fails without that fix.

@MichaelHuth
Copy link
Collaborator Author

MichaelHuth commented Jun 29, 2023

Its way too late here for a proper review but I think that in 5149f3d the indexing of statusTTLFiltered is wrong. It should be by active channel aka i. See how DC_ITC_MakeTTLWave does it in main.

statusTTLFiltered has NUM_DA_TTL_CHANNELS, thus indexes over the channels. Whereas in main the loop in DC_ITC_MakeTTLWave also indexed from 0 to NUM_DA_TTL_CHANNELS after the change I index over the number of active TTL channels (0 to s.numTTLEntries).
The active channel number is retrieved through the config wave by channelNumber = config[i + ttlOffset][%ChannelNumber], that corresponds to iin main.

@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch from ca2b427 to 0e41060 Compare June 29, 2023 05:16
@t-b
Copy link
Collaborator

t-b commented Jun 29, 2023

Its way too late here for a proper review but I think that in 5149f3d the indexing of statusTTLFiltered is wrong. It should be by active channel aka i. See how DC_ITC_MakeTTLWave does it in main.

statusTTLFiltered has NUM_DA_TTL_CHANNELS, thus indexes over the channels. Whereas in main the loop in DC_ITC_MakeTTLWave also indexed from 0 to NUM_DA_TTL_CHANNELS after the change I index over the number of active TTL channels (0 to s.numTTLEntries). The active channel number is retrieved through the config wave by channelNumber = config[i + ttlOffset][%ChannelNumber], that corresponds to iin main.

No, look at the code https://github.com/AllenInstitute/MIES/blob/main/Packages/MIES/MIES_DataConfigurator.ipf#L682 for ITC.

@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch 4 times, most recently from 4e671ae to 34e4c2c Compare June 30, 2023 13:18
@t-b
Copy link
Collaborator

t-b commented Jul 2, 2023

@MichaelHuth How should we proceed here?

@t-b t-b force-pushed the feature/1791-add_ttl_stimsets branch from 77c7336 to 344e5ee Compare July 3, 2023 18:21
@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch 5 times, most recently from 99dce3e to 9bfa7c4 Compare July 4, 2023 18:45
@MichaelHuth MichaelHuth mentioned this pull request Jul 4, 2023
Merged
@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch 2 times, most recently from 04d218d to 46470fc Compare July 6, 2023 03:05
@MichaelHuth
Copy link
Collaborator Author

MichaelHuth commented Jul 6, 2023

@t-b The part of new commits after #1797
starts with 61b4c53 (Tests: Fix UnassociatedChannelsAndTTLs numRacks determination, 2023-06-28)

@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jul 6, 2023
@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch from 46470fc to e772d54 Compare July 6, 2023 13:05
MichaelHuth added 21 commits July 13, 2023 15:03
@MichaelHuth
EP: Add channelType support to EP_AddEpochs
bdcb462 
- supports additionally to DA channels now also TTL channels
- adapt all occurences of EP_AddEpochs
@MichaelHuth
EP: Allow EP_FetchEpochs to retrieve epoch information from TTL channels
137a5b2
@MichaelHuth
EP: Allow TTL channels in EP_GetEpochs
d93a026
@MichaelHuth
EP: Add support for channel type to EP_EpochWaveToStr
b6a433e
@MichaelHuth
EP: Add channelType support to EP_GetEpochCount
1dea597 
- adapt calls in EP_SortEpochs and EP_GetEpochs
@MichaelHuth
EP: Adapt EP_GetEpochs to epochs wave with channel type as chunks
898ec2a 
- needs to be reduced to single channel wave before calling FindIndizes as
  FindIndizes does not support chunks
- name single channel epoch wave consistently "epochInfoChannel"

Adapt test for EP_GetEpochs for assertion on invalid channel type

- TTL channels are now allowed, so we ask for an always invalid channel type
@MichaelHuth
EP: Add TTL channel type support to EP_AddUserEpoch
f197918
@MichaelHuth
EP: Add TTL channel type suport to EP_AdaptEpochInfo
9f88865 
- introduce static function EP_AdaptEpochInfoChannel that adapts the
  epoch information of a single channel
- for DAC channels the hardware channel number from the config wave equals
  the gui channel number, for TTL channels the indexing is over the gui
  channel numbers.
@MichaelHuth
EP: Add channel type support to EP_SortEpochs
4784de8
@MichaelHuth
EP: Adapt AppendLBNEpochs to for new EpochsWave format with TTL chunks
b46b156 
- added early return für empty epochs in EP_FetchEpochs because the loop
  over all TTL channels in AppendLBNEpochs tries also to fetch from
  non-existing epoch channels.
@MichaelHuth
EP: Rename EP_AppendLBNEpochs to EP_CopyLBNEpochsToEpochsWave
9d9fa68 
- more descriptive function name
@MichaelHuth
EP: Write epoch information for TTL and DA channels into LBN
daf844e 
- in EP_WriteEpochInfoIntoSweepSettings
@MichaelHuth
Add entries to sweep text NB for TTL epochs
a7429cb 
- for channel 0 - 7 using the format from CreateTTLChannelLBNKey
- also add description for new entries to description file
- increase SWEEP_SETTINGS_WAVE_VERSION
@MichaelHuth
EP: Move collection of DA epochs to own function EP_CollectEpochInfoDA
1d52042 
- no functional change
- prepares adding a function to collect epoch info for TTL that follows a
  different logic
@MichaelHuth
EP: Remove getting config waveref in EP_CollectEpochInfo
ca3a36f 
- it is not used
- no functional change
@MichaelHuth
EP: Add channel type support to EP_AddEpochsFromStimSetNote
4b81fcf 
- as TTL stimsets do not use a scaling factor, the scale is set to 1
  for TTL channels
@MichaelHuth
EP: Add function EP_CollectEpochInfoTTL that creates epochs for TTL c…
e55bb43 
…hannels

- the epoch is setup the same way as the creation of the DAQDataWave
- EP_AddEpochsFromStimSetNote is used also for TTL epochs

Add epoch test for TTL channels, update TestEpochsGeneric for TTL

- The new test also enables TTL channels
- TestEpochsGeneric was adapted to also check TTL channels for epoch information
- The EpochsTight check was wrapped in a function that takes the channel type
  as argument
@MichaelHuth
EP: Rename constant for baseline trail to EPOCH_SN_BL_GENERALTRAIL
9e53470 
- previously EPOCH_SN_BL_DDAQTRAIL

The new constant name is more descriptive as we use that epoch generically to
fill the time intervall after stim sets in channels. Each channel can have a
stim set of different length or in case of OODAQ even shifted stim sets.
@MichaelHuth
Util: New function GetChannelNumberTTL to convert from hardware to GU…
f43f2f0 
…I channel

- the function takes the hardware channel number and tlBit information and returns
  the GUI channel number for TTL channels
- this commit is a preparation for the following commit, where this function
  is used
@MichaelHuth
BSP: Add TTL channel support to BSP_AddTracesForEpochs
df25647 
- trace infos from associated channels (AD or DA), unassociated DA and TTL
  are collected and concatenated
- renamed channelNumber to hwChannelNumber from traceInfos to emphasize which
  channel number we are working with
- added for distinction guiChannelNumber variable for calls that take the GUI
  channel number
@MichaelHuth
MiesUtil: Add axes for TTL epochs to LayoutGraph function
e6d6f19 
- add new field visualizeEpochs to TiledGraphSettings structure
  to prevent showing TTL epoch axes if the unsplitted TTL trace
  is shown

- epochs are only shown for TTL if splitTTLBits checkbox is activated as well
@MichaelHuth MichaelHuth force-pushed the feature/1791-add_ttl_stimsets branch from 4bdbe06 to e6d6f19 Compare July 13, 2023 13:03
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jul 13, 2023
@t-b t-b self-requested a review July 13, 2023 14:02
@t-b t-b enabled auto-merge July 13, 2023 14:03
@t-b t-b merged commit 8fc9783 into main Jul 13, 2023
@t-b t-b deleted the feature/1791-add_ttl_stimsets branch July 13, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t-b t-b t-b approved these changes

@timjarsky timjarsky timjarsky left review comments

Assignees

@t-b t-b

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add epoch info for TTL stimsets into LBN

4 participants

@MichaelHuth @t-b @timjarsky