Skip to content
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

PSQ_DAScale/PSQ_DS_ADAPT: Fix DAScale estimation after fill-in #2241

Merged
merged 7 commits into from
Sep 18, 2024
Merged

PSQ_DAScale/PSQ_DS_ADAPT: Fix DAScale estimation after fill-in #2241

merged 7 commits into from
Sep 18, 2024

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Aug 21, 2024 •
edited
Loading

  • Why are we acquiring multiple passing sweeps with the same DAScale?
  • TJ provides new data

Close #2224

@t-b t-b assigned t-b and unassigned timjarsky Aug 21, 2024
@t-b t-b force-pushed the bugfix/2241-adaptive-supra-dascale-calculation branch from 11ce187 to 87e194a Compare August 21, 2024 19:26
@t-b t-b assigned timjarsky and unassigned t-b Aug 21, 2024
@t-b

This comment was marked as outdated.

Sign in to view
@timjarsky

This comment was marked as outdated.

Sign in to view
@t-b t-b changed the title PSQ_DAScale/PSQ_DS_ADAPT: Fix DAScale estimation after fillin PSQ_DAScale/PSQ_DS_ADAPT: Fix DAScale estimation after fill-in Aug 22, 2024
@timjarsky timjarsky assigned t-b and unassigned timjarsky Aug 30, 2024
@timjarsky

This comment was marked as outdated.

Sign in to view
@t-b t-b force-pushed the bugfix/2241-adaptive-supra-dascale-calculation branch from 87e194a to 63b05d5 Compare September 3, 2024 07:44
@t-b

This comment was marked as outdated.

Sign in to view
@t-b
Copy link
Collaborator Author

t-b commented Sep 6, 2024

Data from AdaptiveSupraExpWithLatestFix.zip (Date 4.09.24) has multiple MIES versions used:

  Dbh_Cre_KI_Ai65_751022_08_06_03: Release_2.7_20230809-1011-g87e194ac3
  Dbh_Cre_KI_Ai65_751022_08_06_02: Release_2.7_20230809-1011-g87e194ac3
  Dbh_Cre_KI_Ai65_751022_08_06_01: Release_2.7_20230809-1011-g87e194ac3
  C57BL6J_758777_11_04_02: Release_2.7_20230809-38-g89aadde03
  C57BL6J_758777_11_04_01: Release_2.7_20230809-38-g89aadde03
  C57BL6J_758778_12_04_01: Release_2.7_20230809-1011-g87e194ac3
  C57BL6J_755865_06_02_01: Release_2.7_20230809-969-g1a0233d30
  C57BL6J_755865_06_01_01: Release_2.7_20230809-969-g1a0233d30
  QN24_26_014_16_04A_01: Release_2.7_20230809-1011-g87e194ac3
  Dbh_Cre_KI_Ai65_751029_11_06_01: Release_2.7_20230809-969-g1a0233d30;Release_2.7_20230809-1011-g87e194ac3
  Dbh_Cre_KI_Ai65_751029_11_06_02: Release_2.7_20230809-969-g1a0233d30;Release_2.7_20230809-1011-g87e194ac3

this MR is 87e194a (PSQ_DS_GetValuesOfLargestAPFreq: Only fetch passing data, 2024-08-21).

Mapping from sweepbrowser to experiment:

  Window: SweepBrowser8, Title: Browser [8] , Exp: workFolder:Dbh_Cre_KI_Ai65_751022_08_06_03
  Window: SweepBrowser7, Title: Browser [7] , Exp: workFolder:Dbh_Cre_KI_Ai65_751022_08_06_02
  Window: SweepBrowser6, Title: Browser [6] , Exp: workFolder:Dbh_Cre_KI_Ai65_751022_08_06_01
  Window: SweepBrowser5, Title: Browser [5] , Exp: workFolder:C57BL6J_758778_12_04_01
  Window: SweepBrowser4, Title: Browser [4] , Exp: workFolder:C57BL6J_755865_06_02_01
  Window: SweepBrowser3, Title: Browser [3] , Exp: workFolder:C57BL6J_755865_06_01_01
  Window: SweepBrowser2, Title: Browser [2] , Exp: workFolder:QN24_26_014_16_04A_01
  Window: SweepBrowser1, Title: Browser [1] , Exp: workFolder:Dbh_Cre_KI_Ai65_751029_11_06_01
  Window: SweepBrowser, Title: Browser , Exp: workFolder:Dbh_Cre_KI_Ai65_751029_11_06_02

@t-b
Copy link
Collaborator Author

t-b commented Sep 6, 2024

First experiment I'm looking at (arbitrarily selected):

  Window: SweepBrowser5, Title: Browser [5] , Exp: workFolder:C57BL6J_758778_12_04_01;
  C57BL6J_758778_12_04_01: Release_2.7_20230809-1011-g87e194ac3

@t-b

This comment was marked as outdated.

Sign in to view
t-b added 5 commits September 6, 2024 22:07
@t-b
PSQ_DS_GetLabnotebookData: Add PSQ_DS_FI_OFFSET
3f255d4
@t-b
PSQ_DAScale: Fix DAScale estimation for PSQ_DS_ADAPT
68e1ce7 
When we have filled in AP frequencies via PSQ_DS_GatherOvershootCorrection
and don't have any futureDAScales left to measure, we need to calculate a
new DAscale value via PSQ_DS_CalculateDAScale.

But before this commit we did pass in the fI slope/offset, apfreq, DAScale from the just
filled in sweep, which is wrong. We need to pass in the data from the
sweep with the highest AP frequency to continue our search. To
complicate things even more we need to search backwards and use the most
recent sweep.

Bug present since feaf86f (PSQ_DaScale: Add new operation mode Adaptive
Suprathreshold, 2023-10-20).
@t-b
PSQ_DS_GetValuesOfLargestAPFreq: Only fetch passing data
b5471bb 
It makes no difference from the behaviour because we are only called if
the sweep passed and we have no futureDAScale values, but it makes the
code easier to grasp.
@t-b
PSQ_DS_GetValuesOfLargestAPFreq: Fix returned fitSlope
c8d8ef6 
Bug introduced in 68e1ce7 (PSQ_DAScale: Fix DAScale estimation for
PSQ_DS_ADAPT, 2024-08-21).
@t-b
PSQ_DS_GetValuesOfLargestDAScale: Fix it
8bfcc37 
In 68e1ce7 (PSQ_DAScale: Fix DAScale estimation for PSQ_DS_ADAPT,
2024-08-21) we introduced a new way to calculate the new DASCale for
passing sweeps when we have no future DAScale values left.

The idea was sound, but failed to see that we should use the values from
the sweep with the largest DAScale value and not AP freq.
@t-b t-b force-pushed the bugfix/2241-adaptive-supra-dascale-calculation branch from 63b05d5 to 8bfcc37 Compare September 6, 2024 21:00
@t-b

This comment was marked as outdated.

Sign in to view
@t-b

This comment was marked as outdated.

Sign in to view
@t-b t-b assigned timjarsky and t-b and unassigned t-b Sep 9, 2024
t-b added 2 commits September 13, 2024 19:44
@t-b
PSQ_DS_GetLabnotebookKeyConstants: Fix constants for FI-slope
c4099d7 
Broken since 3f255d4 (PSQ_DS_GetLabnotebookData: Add PSQ_DS_FI_OFFSET,
2024-08-21).
@t-b
PSQ_DS_GetLabnotebookData: Enhance function interface
4549ee5 
By accepting labnotebook waves directly we allow to use these functions
also post-mortem on existing data.
@t-b t-b assigned timjarsky and unassigned t-b Sep 13, 2024
@t-b t-b enabled auto-merge September 17, 2024 21:52
@timjarsky timjarsky assigned t-b and unassigned timjarsky Sep 18, 2024
@t-b t-b merged commit 831972d into main Sep 18, 2024
20 checks passed
@t-b t-b deleted the bugfix/2241-adaptive-supra-dascale-calculation branch September 18, 2024 21:33
@t-b t-b mentioned this pull request Sep 23, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timjarsky timjarsky timjarsky approved these changes

@MichaelHuth MichaelHuth Awaiting requested review from MichaelHuth MichaelHuth is a code owner

Assignees

@t-b t-b

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adaptive suprathreshold: Feedback from testing
2 participants
@t-b @timjarsky