Skip to content

Disable global insert TP for unassoc DA channels option #1772

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
Jul 5, 2023
Merged

Disable global insert TP for unassoc DA channels option #1772

t-b merged 8 commits into from
Jul 5, 2023

Conversation

@MichaelHuth
Copy link
Collaborator

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

close #1764

TP is run on associated DA channels only during ITI. No change here regarding unassociated DA channels.

@MichaelHuth MichaelHuth self-assigned this Jun 5, 2023
@MichaelHuth MichaelHuth requested review from t-b and timjarsky as code owners June 5, 2023 22:49
@MichaelHuth MichaelHuth force-pushed the feature/1772-no_tp_for_unassoc_channels_option branch 2 times, most recently from ff15f67 to 03cb482 Compare June 6, 2023 18:16
@MichaelHuth MichaelHuth force-pushed the feature/1772-no_tp_for_unassoc_channels_option branch from 03cb482 to 5ff0c1c Compare June 20, 2023 13:19
@MichaelHuth MichaelHuth changed the base branch from main to feature/1783-add_epoch_info_for_unassoc_DA June 20, 2023 13:20
@MichaelHuth MichaelHuth force-pushed the feature/1772-no_tp_for_unassoc_channels_option branch from e5edceb to c28a84b Compare June 20, 2023 16:21
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 21, 2023
@t-b
Copy link
Collaborator

t-b commented Jun 21, 2023

Review:

Looks mostly really nice. Two minor comments:

  • 24452a4 The commit message should say why this is needed.
  • c28a84b Is adding unrelated changes

I think that the name of the new LBN entry "Global TP insert on unassociated DA channels" is a bit too long, but I leave that bikeshedding to Tim.

@t-b t-b assigned MichaelHuth and unassigned t-b Jun 21, 2023
@MichaelHuth MichaelHuth force-pushed the feature/1772-no_tp_for_unassoc_channels_option branch from c28a84b to 9a27e88 Compare June 22, 2023 02:52
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 22, 2023
@t-b t-b assigned MichaelHuth and unassigned t-b Jun 22, 2023
@MichaelHuth MichaelHuth force-pushed the feature/1783-add_epoch_info_for_unassoc_DA branch 2 times, most recently from f63d55c to 5564138 Compare June 29, 2023 16:16
Base automatically changed from feature/1783-add_epoch_info_for_unassoc_DA to main June 30, 2023 00:03
@t-b
Copy link
Collaborator

t-b commented Jun 30, 2023

@MichaelHuth Needs conflict fixes.

@t-b
Copy link
Collaborator

t-b commented Jun 30, 2023

Please assign me once done.

@MichaelHuth MichaelHuth force-pushed the feature/1772-no_tp_for_unassoc_channels_option branch from 9a27e88 to de79895 Compare June 30, 2023 13:35
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 30, 2023
@t-b
Copy link
Collaborator

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

Review:

d1e949a (DAEphys: Add new checkbox to panel for o TP on unassociated DA channels, 2023-06-05)

Fine.

33e12d4 (DAP: Make CB "TP on unassoc" dependent from Insert TP checkbox, 2023-06-06)

Good.

d422a4e (DAEphys: Increase panel version to 61, 2023-06-05)

  • Merge with d1e949a. This makes it easier to find the new checkbox.

f71ad3d (DC: Add no TP for unassoc DA in DataConfigurationResult structure, 2023-06-05)

Jeep.

2dfcc5c (DC: Dont add TP on Acquisition if TP on unassoc checkbox is unchecked, 2023-06-05)

Jeep.

0905228 (LBN: Add new entry for TP on unassoc DA channels in LBN, 2023-06-06)

Fine.

ac61cb0 (EP: Add epoch for No TP on unassociated DA channels -> B0_TP, 2023-06-20)

Good.

2c3c77d (Tests: Adapt TestEpochsGeneric to test all DA channels, 2023-06-20)

+1

de79895 (Tests: Add epoch test for unassoc DA channel without insert TP, 2023-06-22)

  • The test should also check that the normal TP epochs are missing on the unassoc DA, something like
	WAVE/Z tpMissing = FindIndizes(epochNames2, str="TP*", prop = PROP_WILDCARD)
	CHECK_WAVE(tpMissing, NULL_WAVE)

  • It should also check the new LBN entry.

  • We should also extend another epoch test, EP_EpochTest15?, to check the default behaviour of the new checkbox.

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

t-b commented Jun 30, 2023

0905228 needs to raise LABNOTEBOOK_VERSION.

@MichaelHuth
DAEphys: Add new checkbox to panel for o TP on unassociated DA channels
ff76cac 
- checkbox got a Configuration Restore priority of 60 to be restored before the
  inserted TP checkbox
- increase panel version to 61
@MichaelHuth
DAP: Make CB "TP on unassoc" dependent from Insert TP checkbox
a4f3b82 
- If Insert TP is unselected then TP on unassoc is unselected and disabled
  If Insert TP is selected then TP on unassoc is restored
@MichaelHuth
DC: Add no TP for unassoc DA in DataConfigurationResult structure
343546b 
- and retrieve it in DC_GetConfiguration
@MichaelHuth
DC: Dont add TP on Acquisition if TP on unassoc checkbox is unchecked
d59ad10
@MichaelHuth
LBN: Add new entry for TP on unassoc DA channels in LBN
c06605c 
- Stores the checkbox state of Check_Settings_UnassocDADoTP from DAEphys panel
- key is: TPONUNASSOCDA_ENTRY_KEY
- It is stored in DC_PrepareLBNEntries
- Increased wave version to 37
- Increased LBN version to 70
@MichaelHuth
EP: Add epoch for No TP on unassociated DA channels -> B0_TP
3c4dcb1 
- If the user switches off the TP for unassoc DA channels in range a
  Baseline is present that fills the time where the associated channels run TP.
  For that time interval we need an B0 epoch.
- also add docu for that new epoch
@MichaelHuth
Tests: Adapt TestEpochsGeneric to test all DA channels
9ba175b 
- before it tested only assoc. DA channels
@MichaelHuth
Tests: Add epoch test for unassoc DA channel without insert TP
ec5fad4 
- checks if TP interval of assoc DA equals the B0_TP interval of
  unassoc DA with disabled TP
@MichaelHuth MichaelHuth force-pushed the feature/1772-no_tp_for_unassoc_channels_option branch from de79895 to ec5fad4 Compare June 30, 2023 15:16
@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jun 30, 2023
@t-b
Copy link
Collaborator

t-b commented Jun 30, 2023

Very nice! Thanks!

@t-b t-b assigned timjarsky and unassigned t-b Jun 30, 2023
@t-b t-b mentioned this pull request Jul 3, 2023
Closed
26 tasks
@timjarsky timjarsky assigned t-b and MichaelHuth and unassigned timjarsky Jul 5, 2023
@t-b t-b merged commit 7ca1554 into main Jul 5, 2023
@t-b t-b deleted the feature/1772-no_tp_for_unassoc_channels_option branch July 5, 2023 19:40
@timjarsky
Copy link
Collaborator

timjarsky commented Jul 5, 2023

@MichaelHuth, sorry for this late question, but how are the TP properties (e.g., amplitude) on unassociated DA channels set?

@t-b t-b mentioned this pull request Jul 5, 2023
Closed
@MichaelHuth
Copy link
Collaborator Author

MichaelHuth commented Jul 6, 2023

For inserting the TP in unassociated DA channels the same code as for associated DA channels is run. Thus, the same settings apply for unassociated DA channels.
The checkbox to switch the inserted TP off for unassociated DA replaces the time interval where for associated DA channels the TP is inserted with constant baseline amplitude.

@t-b
Copy link
Collaborator

t-b commented Jul 6, 2023

Let's move the discussion to the issue I created.

@AllenInstitute AllenInstitute locked as resolved and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

@timjarsky timjarsky timjarsky approved these changes

Assignees

@t-b t-b

@MichaelHuth MichaelHuth

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DAEphys: Add checkbox to not insert TP and don't do TP during ITI for unassociated DA

4 participants

@MichaelHuth @t-b @timjarsky