Recreate user epochs for PSQ_Chirp

[t-b]

@MichaelHuth Can you check with the data we have on the FTP in folder issue-881 that the chirp epochs are recreated correctly?

[MichaelHuth]

There seems to be no data file with a stimset where PSQ_Chirp as used.

[t-b]

How have you looked? I found some in C57BL6J-628261.02.01.02.nwb.

[MichaelHuth]

I patched in a fallback for old data and reordered some LNB dependencies. Though, I can not test it for correctness ad the old data has no USER epochs to compare and used an old version of the analysisfunction code. With this the epoch recreation creates more epochs as there were originally present. This raises the issue that the old file has regular epochs, such that by default no epoch recreation kicks in. However, if I enforce epoch recreation manually, I get more epochs out that originally saved.

[t-b]

Though, I can not test it for correctness ad the old data has no USER epochs to compare and used an old version of the analysisfunction code.

I think you need to visually check that the reconstructed user epochs are correct.

This raises the issue that the old file has regular epochs, such that by default no epoch recreation kicks in. However, if I enforce epoch recreation manually, I get more epochs out that originally saved.

This should not be an issue as we want to regenerated all epochs including user epochs.

[t-b]

Review:

• [x] This PR does not close #2074 as it only implements parts of it

a501aa2d3 (EP: Make EP_AddUserEpoch device independent, 2024-04-11)

Jeep.

8ec12641d (Tests: Move helper function for epoch recreation testing to UTF_HelperFunctions, 2024-04-11)

Good.

7769990cb (Ana: refactor - move parts of Chirp user epoch creation to own device independent functions, 2024-04-11)

• [x] Please add a comment at PSQ_CR_AddSpikeEvaluationEpoch/PSQ_CR_AddCycleEvaluationEpoch to say that the returned units are seconds.

bdcb65074 (Ana: Rename totalOnsetDelay in PSQ_CR_AddSpikeEvaluationEpoch to totalOnsetDelayMS, 2024-04-11)

Nice.

e5b3b19e6 (EP: Add EpochRecreation support for User epochs from PSQ_Chirp, 2024-04-11)

• [x] I think the part

added epoch recreation test to PS_CR2, PS_CR12 and PS_CR13

in the commit message does not reflect the code anymore.

• [x] Can we switch to using constants for the analysis function types? MapAnaFuncToConstant should help.
• [x] No need to cite the function name in DEBUGPRINT, it is also wrong.

EP_AddRecreatedUserEpochs_PSQ_Chirp:

• [x] Not being able to query the analysis function parameters should assert out

CommonAnalysisFunctionChecks:

• [x] There should be always a "Generic Function" entry available

• [x] The GetRowIndex call makes me shiver. You already have the analysis function type in type so no need to query the LBN again.

• [x] TestEpochReceationRemoveUserEpochs: Missing r

adcbf417e (EP: Add recreation support for PSQ_Chirp baseline epochs, 2024-04-17)

• [x] "number of evaluated chunk" -> "index of evaluated chunk"
• [x] Don't passt the name of analysis functions around but their numeric type, in TestEpochReceationRemoveUnsupportedUserEpochs
• [x] I know we talked about only removing BLC0 and BLC1, but now that i think about it again I think we should keep all "^BLC\d+$" epochs. Please also make that a strconstant close to PSQ_BASELINE_CHUNK_SHORT_NAME_PREFIX so that we see that both belong together. I'd also appreciate if we could move away from the loop handling here and use FindIndizes and friends.

3288f5dd0 (Tests: Add test for checking user epochs in historic data, 2024-04-20)

• [x] Commit message: " does not has" > "does not have"
• [x] prefer createDFWithAllParents over NewDataFolder
• [x] Why the C&P of code in ExportEpochsFromFileToDFOpenAnalysisBrowserHelper from OpenAnalysisBrowser? I also don't see what ExportEpochsFromFileToDFOpenAnalysisBrowserHelper has to do with epochs. If you want to use OpenAnalysisBrowser outside of test code. Fine let's remove test assertions from OpenAnalysisBrowser.
• [x] WAVE/Z refEpochs = $"" -> "Wave/ZZ refEpochs"
• [x] Please add some documentation for CompareEpochsOfSweep and ExtendRefEpochsWithUserEpochs
• [x] ExtendRefEpochsWithUserEpochs: I don't understand why you check

+        if(strsearch(shortName, EPOCH_SHORTNAME_USER_PREFIX, 0) != 0)
+            continue
+        endif

from the naming epochChannelRecUser only holds user epochs. Maybe this should be an assert?

General comment: If you have test code which does

Function MyTEstFunction()

for(...)

  ...
  CHECK_XXX()
endfor

End

you always need an assertion before the loop that the number of loop iterations is larger than zero. Otherwise you have passing test without checking anything.

• [x] The comment "For user epochs also the values are checked" for CompareEpochsHistoricChannel does not seem to match the code.

cf63db435 (Tests: Split generators in HistoricDataTests for PXP and NWB files, 2024-04-22)

Looks good!

[MichaelHuth]

There should be always a "Generic Function" entry available

There is not always a "Generic Function" entry available, e.g. see the early MIES experiments from historic data.

[MichaelHuth]

I'd also appreciate if we could move away from the loop handling here and use FindIndizes and friends

I rewrote that part, though I think its more complex and less efficient now.

[t-b]

Review:

• [x] PSQ_BASELINE_CHUNK_SHORT_NAME_RE_MATCHER should be anchored to front and end, i.e. ^BLC[[:digit:]]+$ as we don't want to match "aBLC0b".

I've created https://github.com/AllenInstitute/MIES/issues/2095 for the obvious FindIndizes enhancement and https://github.com/AllenInstitute/MIES/issues/2096 for DeleteWavePoint.

Rest looks good.

Edit: I think PSQ_BASELINE_CHUNK_SHORT_NAME_RE_MATCHER should then also contain the user prefix. WE can't change that anyway.

[MichaelHuth]

Ok I removed the generic assembly of the regex in favor of including it directly in the strings.