Skip to content

Add epoch information for unassociated DA channels + display in DB #1783

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 8 commits into from
Jun 30, 2023
Merged

Add epoch information for unassociated DA channels + display in DB #1783

t-b merged 8 commits into from
Jun 30, 2023

Conversation

@MichaelHuth
Copy link
Collaborator

@MichaelHuth MichaelHuth commented Jun 12, 2023 •
edited
Loading

Checked:

  • EP_CollectEpochInfo: works on basis of DA channels and already collects unassoc DA
  • EP_FetchEpochs: works on all DA channels
  • NWB_AppendSweepLowLevel: Saves already unassoc. DA channel epoch info L1232 and L1243
  • TestNwbExportv2: Tests already unassoc. DA channels as well L528 ff.

close #1765

@MichaelHuth MichaelHuth requested a review from timjarsky as a code owner June 12, 2023 16:38
@MichaelHuth MichaelHuth self-assigned this Jun 12, 2023
@MichaelHuth MichaelHuth requested a review from t-b as a code owner June 12, 2023 16:38
@MichaelHuth MichaelHuth force-pushed the feature/1783-add_epoch_info_for_unassoc_DA branch 2 times, most recently from 84988fc to 85a987d Compare June 13, 2023 14:35
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 13, 2023
@t-b t-b mentioned this pull request Jun 13, 2023
Merged
3 tasks
@t-b
Copy link
Collaborator

t-b commented Jun 13, 2023 •
edited by MichaelHuth
Loading

  • Write test for SF epochs/data

@t-b t-b assigned MichaelHuth and unassigned t-b Jun 13, 2023
@t-b
Copy link
Collaborator

t-b commented Jun 14, 2023

I'm doing a review now so that you can incorporate the feedback when writing the tests:
de85bb0 (EP: EP_CollectEpochInfo remove headstage variable as it was unused, 2023-06-12)

Nice find.

c5bfdbd (BSP: BSP_AddTracesForEpochs add support for unassoc DA channel epochs, 2023-06-12)

That's is a hodge-podge of changes.
The change from GetLastSetting to EP_FetchEpochs is a good one. But this should be it's one commit, and also handle the case where EP_FetchEpochs returns $"" and also keep the "present since" comment. Doing that as an upfront change can be motivated in the commit message with "In a future commit we want to ...".
I'm fine with introducing DB_AXIS_PART_EPOCHS but in it's own commit upfront and also replace all occurences.

// epoch info for associated DA channels this comment is now wrong.

bf7e0c4 (BSP: BSP_AddTracesForEpochs, put trace creation in a loop, 2023-06-12)
84988fc (BSP: BSP_AddTracesForEpochs correct loop vars j, k -> i, j, 2023-06-12)

Both are good.

@MichaelHuth MichaelHuth force-pushed the feature/1783-add_epoch_info_for_unassoc_DA branch 2 times, most recently from b1896b1 to 25ae947 Compare June 15, 2023 21:40
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 15, 2023
@MichaelHuth MichaelHuth force-pushed the feature/1783-add_epoch_info_for_unassoc_DA branch from 25ae947 to 5a5e9a6 Compare June 16, 2023 00:16
@t-b
Copy link
Collaborator

t-b commented Jun 16, 2023

Very nice! Also the test is succinct and complete!

@t-b t-b assigned timjarsky and unassigned t-b Jun 16, 2023
timjarsky
timjarsky previously approved these changes Jun 28, 2023
View reviewed changes
@timjarsky timjarsky assigned t-b and MichaelHuth and unassigned timjarsky Jun 28, 2023
@t-b
Copy link
Collaborator

t-b commented Jun 28, 2023

@MichaelHuth Please rebase and fix the conflict. I'll merge once CI passes.

@t-b t-b removed their assignment Jun 28, 2023
@MichaelHuth MichaelHuth force-pushed the feature/1783-add_epoch_info_for_unassoc_DA branch from 5a5e9a6 to f63d55c Compare June 29, 2023 16:15
@MichaelHuth
BSP: In BSP_AddTracesForEpochs use EP_FetchEpochs
1931553 
Preparation for unassoc DA channel support:
previously GetLastSetting was called that supports only associated channels,
now EP_FetchEpochs is called that relies only on the DA channel.
If the displayed channel is an AD channel the DA channel is determined through
the headstage relation.
@MichaelHuth
BSP: BSP_AddTracesForEpochs add support for unassoc DA channel epochs
0566add 
- get also unassoc DA channels from traceInfos
- use more generic string for unique name of displayed epoch waves
- also consider unassic DA channel traces in LayoutGraph
@MichaelHuth
SF: Adapt SFH_GetSweepsForFormula DA channel resolve for epoch retrieval
fc04d7d 
- SFH_GetSweepsForFormula tried to resolve the DA channel for epoch retrieval
  always through SFH_GetDAChannel.
  That function only supports associated channels and it i sonly senseful to
  call for AD channels or to verify a channelNumber for DA.
-> It is now only called to resolve AD to DA channels.
@MichaelHuth
Test: Add test for operation data/epochs for unassoc DA
037975b 
- checks if data and epochs operation retrievel data refered by
  epoch short names correctly from unassociated DA channels
@MichaelHuth
BSP: BSP_AddTracesForEpochs, put trace creation in a loop
c64c94e 
- shorter code
@MichaelHuth
BSP: BSP_AddTracesForEpochs correct loop vars j, k -> i, j
5564138 
- according to coding conventions and nesting order
@MichaelHuth MichaelHuth force-pushed the feature/1783-add_epoch_info_for_unassoc_DA branch from f63d55c to 5564138 Compare June 29, 2023 16:16
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 29, 2023
@t-b t-b enabled auto-merge June 29, 2023 16:33
@t-b t-b merged commit 406440a into main Jun 30, 2023
@t-b t-b deleted the feature/1783-add_epoch_info_for_unassoc_DA branch June 30, 2023 00:03
@t-b t-b mentioned this pull request Apr 2, 2025
Open
4 tasks
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 unassociated DA channels

4 participants

@MichaelHuth @t-b @timjarsky