Conversation
75c4b24 to
bd0beed
Compare
|
@timjarsky This is a first version to play around. There are still a few things I have to add, that I discuss farther below.
xaxisOffset and yaxisOffset are strings that can be General Plotting BehaviorThe operation itself returns internally a full plotting specification that is inserted by the formula plotter at the location where the operation appears in the notebook code. The operation creates only traces that are separated by
Thus, the plotter applies the 10% for x and y-axis when used like this, where the formula setting the plot properties is last in the chain: but not for this: because Operation ArgumentsCurrently:
with The (later) final arguments should also expose arguments from The default for On the basis of the experiment avgMethodTesting2.pxp the generated code is: I added a
for
for I need to add a Therefore, the An additional task from the issue is to add a variable that contains the names of the experiments. I can create this variable in the operation and add it to the variable storage of the formula notebook. It would be available then after the operation ran. |
|
@MichaelHuth attempting to use ivscc_apfrequency() on the linked data , resulted in the following assertions: |
|
I had to fill a request form from Google for the data. |
bd0beed to
d228f41
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new ivscc_apfrequency operation to the sweep formula system, along with supporting infrastructure changes including a flatten operation and refactored plotting code.
Key Changes
- Adds
ivscc_apfrequencyoperation for analyzing action potential frequency in IV sweep current clamp experiments - Introduces
flattenhelper operation to convert datasets of single values into 1D arrays - Refactors plotting infrastructure by introducing
SF_PlotterGraphStructto encapsulate plotting state - Adds axis offset and percentage control capabilities to the plotting metadata system
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| Packages/MIES/MIES_Constants.ipf | Adds new metadata constants for plot customization and operation names (contains merge conflict) |
| Packages/MIES/MIES_SweepFormula_Operations.ipf | Implements flatten and ivscc_apfrequency operations |
| Packages/MIES/MIES_SweepFormula_Executor.ipf | Registers new operations in the executor switch statement |
| Packages/MIES/MIES_SweepFormula.ipf | Major refactoring of plotting code to use structure-based state management |
| Packages/MIES/MIES_SweepFormula_Helpers.ipf | Adds helper functions for internal formula execution and variable storage |
| Packages/MIES/MIES_WaveDataFolderGetters.ipf | Expands plot metadata wave to include axis offset/percentage properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d228f41 to
5e60339
Compare
|
@timjarsky I fixed the assertion that came up. The reason was that with the three experiments the selections result in 20 sweeps from the first experiment, 24 from the second and 16 from the third. The avg operation was transferring the wave notes and meta data always from the first group to the averaged result. And this failed because the averaged result always has the highest number of datasets (24 in this case) and I can not transfer from 20 datasets to 24 datasets as for the last 4 there would be not wave notes and meta data to transfer. For this data this also means that for the first 16 sweeps in the groups the average is over 3 sweeps, 16 - 19 over 2 sweeps and 20 - 23 over 1 sweep. In the most recent version of this PR also the And the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
Packages/MIES/MIES_SweepFormula.ipf:1630
- Missing return statement: The function SF_AddPlotTraceStyle declares a return value of type SF_PlotterGraphStruct but has no explicit return statement. This will cause a compilation error or undefined behavior. Add 'return [pg]' at the end of the function.
static Function [STRUCT SF_PlotterGraphStruct pg] SF_AddPlotTraceStyle(variable formulasAreDifferent)
variable i, j, numTraces, markerCode, lineCode, isCategoryAxis, tagCounter, lineStyle, overrideMarker, traceToFront
string trace, info, tagText, name, wvName
for(i = 0; i < pg.formulaCounter; i += 1)
WAVE/WAVE plotFormData = pg.collPlotFormData[i]
WAVE/T tracesInGraph = plotFormData[0]
WAVE/WAVE dataInGraph = plotFormData[1]
numTraces = DimSize(tracesInGraph, ROWS)
markerCode = formulasAreDifferent ? i : 0
markerCode = SFH_GetPlotMarkerCodeSelection(markerCode)
lineCode = formulasAreDifferent ? i : 0
lineCode = SFH_GetPlotLineCodeSelection(lineCode)
for(j = 0; j < numTraces; j += 1)
WAVE/Z wvX = dataInGraph[j][%WAVEX]
WAVE wvY = dataInGraph[j][%WAVEY]
trace = tracesInGraph[j]
info = AxisInfo(pg.win, "left")
isCategoryAxis = (NumberByKey("ISCAT", info) == 1)
if(isCategoryAxis)
WAVE traceColorHolder = wvX
else
WAVE traceColorHolder = wvY
endif
WAVE/Z traceColor = JWN_GetNumericWaveFromWaveNote(traceColorHolder, SF_META_TRACECOLOR)
if(WaveExists(traceColor))
switch(DimSize(traceColor, ROWS))
case 3:
ModifyGraph/W=$pg.win rgb($trace)=(traceColor[0], traceColor[1], traceColor[2])
break
case 4:
ModifyGraph/W=$pg.win rgb($trace)=(traceColor[0], traceColor[1], traceColor[2], traceColor[3])
break
default:
FATAL_ERROR("Invalid size of trace color wave")
endswitch
endif
tagText = JWN_GetStringFromWaveNote(wvY, SF_META_TAG_TEXT)
if(!IsEmpty(tagText))
name = "tag" + num2str(tagCounter++)
Tag/C/N=$name/W=$pg.win/F=0/L=0/X=0.00/Y=0.00 $trace, 0, tagText
endif
ModifyGraph/W=$pg.win mode($trace)=SF_DeriveTraceDisplayMode(wvX, wvY)
lineStyle = JWN_GetNumberFromWaveNote(wvY, SF_META_LINESTYLE)
if(IsValidTraceLineStyle(lineStyle))
ModifyGraph/W=$pg.win lStyle($trace)=lineStyle
elseif(formulasAreDifferent)
ModifyGraph/W=$pg.win lStyle($trace)=lineCode
endif
WAVE/Z customMarkerAsFree = JWN_GetNumericWaveFromWaveNote(wvY, SF_META_MOD_MARKER)
if(WaveExists(customMarkerAsFree))
DFREF dfrWork = SFH_GetWorkingDF(pg.graph)
wvName = "customMarker_" + NameOfWave(wvY)
WAVE customMarker = MoveFreeWaveToPermanent(customMarkerAsFree, dfrWork, wvName)
ASSERT(DimSize(wvY, ROWS) == DimSize(customMarker, ROWS), "Marker size mismatch")
ModifyGraph/W=$pg.win zmrkNum($trace)={customMarker}
else
overrideMarker = JWN_GetNumberFromWaveNote(wvY, SF_META_MOD_MARKER)
if(!IsNaN(overrideMarker))
markerCode = overrideMarker
endif
ModifyGraph/W=$pg.win marker($trace)=markerCode
endif
traceToFront = JWN_GetNumberFromWaveNote(wvY, SF_META_TRACETOFRONT)
traceToFront = IsNaN(traceToFront) ? 0 : !!traceToFront
if(traceToFront)
ReorderTraces/W=$pg.win _front_, {$trace}
endif
endfor
endfor
End
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0b71dcf to
dc98aba
Compare
|
Thanks for handling the metadata management for mismatched sweep numbers across experiments. A few points for discussion:
|
dc98aba to
79f149a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
79f149a to
95732dc
Compare
|
@timjarsky I have added support for the apfrequency argument block after the first four argument for ivscc_apfrequency. The arguments are now: The last four arguments are "forwarded" to apfrequency. |
|
@MichaelHuth, Are failing sweeps included or filtered out? If included, please update to use only passing sweeps. |
- use *LP_Rheo* instead of *rheo* as stimset selection - better resilience where partial results are zero sized datasets
Adds rank-based binning across experiments - Sort points within each experiment by x (current), low to high. - Build bins by index: bin 1 contains the lowest-x point from each experiment, bin 2 contains the next-lowest point from each experiment, etc. - This yields ~1 point per experiment per bin (e.g., 6 experiments → up to 6 points per bin). - Stopping rule - Stop creating bins once a bin would contain fewer than 2 total data points (i.e., if fewer than 2 experiments contribute a point at that rank). output: Bin y-value = mean frequency across the points in the bin with number of bins datasets, each dataset has a single number mean current across the points in the bin set as x-value in the wave note standard deviation in x and y from each bin is stored as error bar information in the wave note
The PrepareFit operation allows to gather information for fitting input data. The function returns a wave reference wave that stores all gathered information. The format of this wave is designed to allow further extensions for e.g. masking, weights etc. This is a preparation operation for a fit operation that implements the actual fit. PrepareFit allows to use the same fit setup for different fits.
- this allows the user to use getmeta($ivscc_apfrequency_fit)
13a200c to
c855eb2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WAVE/Z/T keys = JWN_GetKeys(wv, "") | ||
| CHECK_EQUAL_TEXTWAVES(keys, {"a", "b"}) | ||
|
|
||
| WAVE/Z/T keys = JWN_GetKeys(wv, "I_DONT_EXIST") |
There was a problem hiding this comment.
This test redeclares the local variable keys a second time (WAVE/Z/T keys = ...) in the same function scope, which is not allowed and will cause a compile error. Reuse the existing keys variable (assignment without redeclaration) or use a different variable name for the second call.
| WAVE/Z/T keys = JWN_GetKeys(wv, "I_DONT_EXIST") | |
| keys = JWN_GetKeys(wv, "I_DONT_EXIST") |
| StrConstant SF_OP_MERGE = "merge" | ||
| StrConstant SF_OP_FIT = "fit" | ||
| StrConstant SF_OP_FITLINE = "fitline" | ||
| StrConstant SF_OP_DATASET = "dataset" |
There was a problem hiding this comment.
SF_OP_MERGE, SF_OP_FIT, SF_OP_FITLINE, and SF_OP_DATASET are defined twice in this block, which will cause a compile-time error due to duplicate StrConstant definitions. Remove the duplicated second set (lines 2577–2580) and keep only a single definition per operation constant.
| StrConstant SF_OP_MERGE = "merge" | |
| StrConstant SF_OP_FIT = "fit" | |
| StrConstant SF_OP_FITLINE = "fitline" | |
| StrConstant SF_OP_DATASET = "dataset" |
| variable numArgs, numCoefs | ||
| string fitfuncName, holdStr, checkStr | ||
|
|
||
| SFH_CheckArgumentCount(exd, opShort, 0, maxArgs = 4) |
There was a problem hiding this comment.
preparefit is documented/implemented to accept up to 5 arguments (including constraints at position 4), but SFH_CheckArgumentCount(..., maxArgs = 4) rejects any call that supplies the constraints argument. Update the max argument count to allow the constraints parameter.
| SFH_CheckArgumentCount(exd, opShort, 0, maxArgs = 4) | |
| SFH_CheckArgumentCount(exd, opShort, 0, maxArgs = 5) |
| WAVE/Z/T keys = JWN_GetKeysAt(data, SF_SERIALIZE) | ||
| serStr = "" | ||
| for(string key : keys) | ||
| path = SF_SERIALIZE + "/" + key | ||
| val = JWN_GetNumberFromWaveNote(data, path) | ||
| if(!IsNaN(val)) | ||
| serStr += key + ": " + num2str(val, "%f") + "\r" | ||
| continue | ||
| endif | ||
| str = JWN_GetStringFromWaveNote(data, path) | ||
| if(!IsEmpty(str)) | ||
| serStr += key + ":\r" + str + "\r" | ||
| continue | ||
| endif | ||
| WAVE/Z wvn = JWN_GetNumericWaveFromWaveNote(data, path) | ||
| if(WaveExists(wvn)) | ||
| str = NumericWaveToList(wvn, "\r") | ||
| serStr += key + ":\r" + str | ||
| continue | ||
| endif | ||
| WAVE/Z/T wt = JWN_GetTextWaveFromWaveNote(data, path) | ||
| if(WaveExists(wt)) | ||
| str = TextWaveToList(wt, "\r") | ||
| serStr += key + ":\r" + str | ||
| continue | ||
| endif | ||
| endfor |
There was a problem hiding this comment.
SFO_OperationGetMeta calls JWN_GetKeysAt(...), but there is no such function added/defined in the repo (the new helper is JWN_GetKeys). This will fail to compile; replace this call with the correct API (and handle the null-wave case if the path does not exist).
| WAVE/Z/T keys = JWN_GetKeysAt(data, SF_SERIALIZE) | |
| serStr = "" | |
| for(string key : keys) | |
| path = SF_SERIALIZE + "/" + key | |
| val = JWN_GetNumberFromWaveNote(data, path) | |
| if(!IsNaN(val)) | |
| serStr += key + ": " + num2str(val, "%f") + "\r" | |
| continue | |
| endif | |
| str = JWN_GetStringFromWaveNote(data, path) | |
| if(!IsEmpty(str)) | |
| serStr += key + ":\r" + str + "\r" | |
| continue | |
| endif | |
| WAVE/Z wvn = JWN_GetNumericWaveFromWaveNote(data, path) | |
| if(WaveExists(wvn)) | |
| str = NumericWaveToList(wvn, "\r") | |
| serStr += key + ":\r" + str | |
| continue | |
| endif | |
| WAVE/Z/T wt = JWN_GetTextWaveFromWaveNote(data, path) | |
| if(WaveExists(wt)) | |
| str = TextWaveToList(wt, "\r") | |
| serStr += key + ":\r" + str | |
| continue | |
| endif | |
| endfor | |
| WAVE/Z/T keys = JWN_GetKeys(data, SF_SERIALIZE) | |
| serStr = "" | |
| if(WaveExists(keys)) | |
| for(string key : keys) | |
| path = SF_SERIALIZE + "/" + key | |
| val = JWN_GetNumberFromWaveNote(data, path) | |
| if(!IsNaN(val)) | |
| serStr += key + ": " + num2str(val, "%f") + "\r" | |
| continue | |
| endif | |
| str = JWN_GetStringFromWaveNote(data, path) | |
| if(!IsEmpty(str)) | |
| serStr += key + ":\r" + str + "\r" | |
| continue | |
| endif | |
| WAVE/Z wvn = JWN_GetNumericWaveFromWaveNote(data, path) | |
| if(WaveExists(wvn)) | |
| str = NumericWaveToList(wvn, "\r") | |
| serStr += key + ":\r" + str | |
| continue | |
| endif | |
| WAVE/Z/T wt = JWN_GetTextWaveFromWaveNote(data, path) | |
| if(WaveExists(wt)) | |
| str = TextWaveToList(wt, "\r") | |
| serStr += key + ":\r" + str | |
| continue | |
| endif | |
| endfor | |
| endif |
| sprintf expr, "selexpAD%d = select(selexp(\"%s\"), $sel, selchannels(AD0), selrange(E1))", i, uniqueFiles[i] | ||
| formula = SF_AddExpressionToFormula(formula, expr) | ||
| sprintf expr, "selexpDA%d = select(selexp(\"%s\"), $sel, selchannels(DA0), selrange(E1))", i, uniqueFiles[i] | ||
| formula = SF_AddExpressionToFormula(formula, expr) | ||
| sprintf expr, "freq%d = apfrequency(data($selexpAD%d), %d, %f, %s, %s, %s)", i, i, method, level, timeFreq, normalize, xAxisType | ||
| formula = SF_AddExpressionToFormula(formula, expr) | ||
| sprintf expr, "current%d = max(data($selexpDA%d))", i, i | ||
| formula = SF_AddExpressionToFormula(formula, expr) | ||
| if(!CmpStr(xaxisOffset, SF_OP_IVSCCAPFREQUENCY_FIRST)) | ||
| sprintf expr, "currentNorm%d = $current%d - extract($current%d, 0)", i, i, i | ||
| elseif(!CmpStr(xaxisOffset, SF_OP_IVSCCAPFREQUENCY_MIN)) | ||
| sprintf expr, "currentNorm%d = $current%d - min(merge($current%d))", i, i, i | ||
| elseif(!CmpStr(xaxisOffset, SF_OP_IVSCCAPFREQUENCY_MAX)) | ||
| sprintf expr, "currentNorm%d = $current%d - max(merge($current%d))", i, i, i | ||
| else // SF_OP_IVSCCAPFREQUENCY_NONE | ||
| sprintf expr, "currentNorm%d = $current%d", i, i | ||
| endif | ||
| formula = SF_AddExpressionToFormula(formula, expr) | ||
| endfor | ||
|
|
||
| Make/FREE/T/N=(numExp) freqs, currents, exps | ||
| freqs[] = "$freq" + num2istr(p) | ||
| freqList = TextWaveToList(freqs, ",", trailSep = 0) | ||
| currents[] = "$currentNorm" + num2istr(p) | ||
| currentList = TextWaveToList(currents, ",", trailSep = 0) | ||
| exps[] = "\"" + uniqueFiles[p] + "\"" | ||
| expList = TextWaveToList(exps, ",", trailSep = 0) |
There was a problem hiding this comment.
SFO_OperationIVSCCApFrequency constructs a SweepFormula string by interpolating experiment file names from uniqueFiles directly into expressions like selexp("%s") and into the ivscc_apfrequency_explist array without any escaping. On platforms where file names can contain " or other special characters, an attacker who controls experiment file names (e.g. via a crafted NWB/PXP on disk) can inject additional SweepFormula operations by breaking out of the quoted string (for example with a name like evil");otherOp()), which will then be executed under the user’s context. To avoid this injection vector, validate file/experiment names to a strict safe character set or escape them appropriately for the SweepFormula string literal context before inserting them into formula via sprintf/SF_AddExpressionToFormula.



close #2581
close #2628