Various PSX fixes #2362

t-b · 2025-02-28T17:55:34Z

FFT size fix using 2^l + 3^k logic, closes: PSX: Make it faster #1663
psx accepted average fix: return real amplitude for double expontential
average fit for reject, undet, all, add into column on the right
Add default values for filter frequencies:

sweepFilter: Default values for low/high, order defaults to 4
low cutoff  = 1 / (3 * pi * riseTau)
high cutoff = 1 / (3 * pi * decayTau)
deconvFilter: Default values for low/high, order defaults to 4
low cutoff  = 1 / (4 * pi * riseTau)
high cutoff = 1 / (4 * pi * decayTau)

Old Todos

So this should be ready for a first test. I've adapted `psxKernel` and `psxStats` to allow passing in multiple selections.

# psxSweepFilter(lowFreq, highFreq [, order = 6])
# back down on having nonfinite values in filtered data, order -2, output passing order
# back down on deconv order as well, same approach
# use code from Tim to determine the highest possible, reapply that order to all sweep data

Output all x/y axis labels for multiple operations with with, see SweepFormula: Groups multiple x-axis labels #2423
Later: Decide later on about support for multiple properties in psxStats, see psxstats: Should accept an array of properties #2422
SweepFormula: Groups multiple x-axis labels #2423
UpdateInfoButtonHelp: Set machine-readable text for C&P #2424

timjarsky · 2025-04-08T20:03:51Z

@t-b I also tested pasting the average fit results into Excel. The row formatting is correct. However, Excel plots all the columns in one column. Excel is dumb. To correct this, paste into Excel, select the cells, select Data from the menu, select Text to Columns, and select the appropriate delimiter in the Excel pop-up GUI.

timjarsky · 2025-04-08T20:38:31Z

@t-b there may also be issues with the indivdual decay tau values. For example in this case I'm getting a negative weighted tau.

These routines allow to calculate all values between 2^1 and 2^30 which have 2 and 3 as prime factors. This helps in deciding a good cut off value for FFT as it only performs when the primes are smaller than 5.

The FFT implementation in Igor Pro requires the prime factors of the length to be < 5 for speedy execution. For some cases these prime factors can be very large (~1e5) and thus FFT would be dead slow, i.e. in the order of minutes on common machines. We now employ a data shortening to a size which has only 2 and 3 as prime factors, which makes FFT very fast. This is only done if we have prime factors larger than a 1000 in the original size.

We now reserve space for the defaults in the psxDeconvBPFilter/psxSweepBPFilter operations so that we can later on add the calculated defaults, which need parameters from psxKernel, can be added via PSX_AddDefaultFilterFrequencies.

And allow querying them from psxstats.

This is convenient for documentation purposes although this is not an input parameter.

Ever since 7aa2be6 (PSX: Add average fit for all states, 2025-06-04) we are using the per-combo index for gathering the eventKernelAmp/eventOnsetTime/eventPeakTime values. This is wrong as we must use the index of the displayed events.

…ge waveform

camel case fix Co-authored-by: Copilot <[email protected]>

timjarsky · 2025-12-03T23:13:27Z

@t-b I'm trying to test this PR, but getting confused. I recall that the most recent version of the PSX operation took several other operations as input arguments. In particular, psxsweepBPfilter, which seems to no longer be present in either active PSX branch.

t-b · 2025-12-03T23:48:18Z

@timjarsky I'll have a look tomorrow.

t-b · 2025-12-10T14:11:00Z

@timjarsky I found something please try again.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-10T14:16:25Z

Packages/MIES/MIES_SweepFormula_PSX.ipf

+	KillWaves/Z root:PSX_hist, root:PSX_histX
+	Make/O/D/N=(nBins) root:PSX_hist, root:PSX_histX
+	WAVE hist  = root:PSX_hist
+	WAVE histX = root:PSX_histX


This function creates and stores waves in the root folder (root:PSX_hist, root:PSX_histX) without cleaning them up afterward. If showPlot is false (the default), these waves are created but never used again, leading to memory leaks. Consider either: 1) Using FREE waves when showPlot=0, or 2) Cleaning up these waves before returning, or 3) Moving them to a dedicated data folder with proper lifecycle management.

Suggested change

KillWaves/Z root:PSX_hist, root:PSX_histX

Make/O/D/N=(nBins) root:PSX_hist, root:PSX_histX

WAVE hist = root:PSX_hist

WAVE histX = root:PSX_histX

if (showPlot)

KillWaves/Z root:PSX_hist, root:PSX_histX

Make/O/D/N=(nBins) root:PSX_hist, root:PSX_histX

WAVE hist = root:PSX_hist

WAVE histX = root:PSX_histX

else

Make/FREE/D/N=(nBins) hist, histX

endif

Copilot · 2025-12-10T14:16:25Z

Packages/MIES/MIES_SweepFormula_PSX.ipf

+Function GetKernelDecayTau()
+
+	string psxPath
+	string ListOfwin = WinList("*", ";", "WIN:64") // Only graph windows
+	variable i
+	variable num = ItemsInList(ListOfwin, ";")
+
+	// Search all graph windows for one with psxFolder user data
+	for(i = 0; i < num; i += 1)
+		string baseWin = StringFromList(i, ListOfwin, ";")
+		psxPath = GetUserData(baseWin, "", "psxFolder")
+		if(strlen(psxPath) > 0)
+			break
+		endif
+	endfor
+
+	if(strlen(psxPath) == 0)
+		printf "No psxFolder user data found in any graph window.\r"


The function GetKernelDecayTau() searches through all graph windows to find PSX data. This is a fragile design that: 1) May return incorrect data if multiple PSX analyses are open, 2) Couples unrelated code through global state, 3) Makes debugging difficult. Consider passing the necessary context (window name or data folder reference) as a parameter instead of searching globally.

Suggested change

Function GetKernelDecayTau()

string psxPath

string ListOfwin = WinList("*", ";", "WIN:64") // Only graph windows

variable i

variable num = ItemsInList(ListOfwin, ";")

// Search all graph windows for one with psxFolder user data

for(i = 0; i < num; i += 1)

string baseWin = StringFromList(i, ListOfwin, ";")

psxPath = GetUserData(baseWin, "", "psxFolder")

if(strlen(psxPath) > 0)

break

endif

endfor

if(strlen(psxPath) == 0)

printf "No psxFolder user data found in any graph window.\r"

/// @brief Returns the kernel decay tau from the given PSX data folder path.

/// @param psxPath The path to the PSX data folder (string).

Function GetKernelDecayTau(psxPath)

string psxPath

if(strlen(psxPath) == 0)

printf "No psxFolder path provided to GetKernelDecayTau.\r"

Copilot · 2025-12-10T14:16:25Z

Packages/MIES/MIES_SweepFormula_PSX.ipf

+	duplicate/O AvgEventDiff, root:forDisp
+	smooth 500, root:forDisp


The code contains debugging/development artifacts (creating root:forDisp on line 6464, commented-out line 6740 about plotting). These should be removed or placed behind conditional compilation flags for production code.

Suggested change

duplicate/O AvgEventDiff, root:forDisp

smooth 500, root:forDisp

Copilot · 2025-12-10T14:16:26Z

Packages/tests/Basic/UTF_SweepFormula_PSX.ipf

 	INFO("index = %d, V_numNaNs = %d, kernelAmpSign = %d", n0 = index, n1 = V_numNans, n2 = kernelAmpSign)

-	// 1 NaN for the first event only, the rest is onset Time
+	// 5 NaNs for the first event only, the rest is onset Time


The comment indicates "5 NaNs for the first event" but the test was changed from expecting 1 NaN to expecting 5 NaNs. This suggests that additional NaN values are being introduced in the event data structure. While this might be intentional due to the addition of new tau fields (weightedTau, slowTau, fastTau) and onset/rise y-values, the comment should be more specific about which fields contain NaNs and why this is expected behavior for the first event.

Suggested change

// 5 NaNs for the first event only, the rest is onset Time

// For the first event, 5 fields are expected to be NaN: weightedTau, slowTau, fastTau, onsetY, and riseY.

// This is because these fields are not defined for the first event due to lack of preceding data or fit results.

Copilot · 2025-12-10T14:16:26Z

Packages/doc/SweepFormula.rst

+   defaults to `NaN`
+
+The default values of `NaN` are replaced inside `psx`. For the order this is
+`7`, for the frequencies `500` (`lowFreq`) and `50` (`highFreq`).


The documentation states that the filter uses default values of 7 for order and 500/50 for frequencies, but the code in MIES_Constants.ipf shows PSX_DECONV_FILTER_DEF_ORDER = 4 (line 2415) and there are no constants defined for default frequencies. The documentation should match the actual default values used in the implementation.

Suggested change

`7`, for the frequencies `500` (`lowFreq`) and `50` (`highFreq`).

`4`. There are no default frequencies defined in the implementation.

Copilot · 2025-12-10T14:16:27Z

Packages/doc/SweepFormula.rst

 The default values of `NaN` are replaced inside `psx`. For the order this is
 `7`, for the frequencies `500` (`lowFreq`) and `50` (`highFreq`).
 Here `lowFreq` is the end and `highFreq` the start of the
-passband, see also the description of `/LO` and `/HI` from `FilterIIR`.
+passband, see also the description of `/LO` and `/HI` from `FilterIIR`. If the
+frequency values are not ordered correctly, they are swapped.


The documentation for psxSweepBPFilter is identical to psxDeconvBPFilter documentation, including the same default values. However, since these are two different filters (sweep vs deconvolution), they may have different default values based on the PR description. The documentation should accurately reflect the defaults for each filter type or note if they are intentionally the same.

Copilot · 2025-12-10T14:16:27Z

Packages/doc/SweepFormula.rst

+   psxDeconvBPFilter(800, 100)
+   psxDeconvBPFilter(400, 50, 11)


The example code still references psxDeconvFilter instead of the new psxDeconvBPFilter function name. This is inconsistent with the renamed operation and will cause errors if users copy this example.

Copilot · 2025-12-10T14:16:27Z

Packages/MIES/MIES_SweepFormula_PSX.ipf

+	//	FindLevel/EDGE=(edge)/Q average, (riselowerThreshold * extrema)
+	variable onsetX = PSX_CalculateOnsetTimeFromAvg(average, meanKernelAmp, meanOnsetTime, meanPeakTime)
+	assert(!isNaN(onsetX), "onset time calculation must cannot be Nan")
+	variable level = (riseLowerThreshold * (extrema - average(onsetX))) + average(onsetX)


The variable name riseLowerThreshold is used here (line 2588) but was defined earlier as riselowerThreshold (line 2583). Igor Pro is case-insensitive for variable names, but this inconsistency in capitalization reduces code readability and maintainability. Consider using consistent capitalization throughout the function.

Suggested change

variable level = (riseLowerThreshold * (extrema - average(onsetX))) + average(onsetX)

variable level = (riselowerThreshold * (extrema - average(onsetX))) + average(onsetX)

timjarsky · 2025-12-16T19:27:06Z

With the file located here, the following SF code doesn't plot anything or show any errors.

sel=select(selchannels(AD09), selvis(all), selCM(VC), selrange([2000,inf]), selsweeps(10...15))


SD=2.5
TauRise = 0.5
TauDecay = 3
Amp = -10

Kernel = PSXKernel($sel, $TauRise, $TauDecay, $Amp)

Doit = PSX(X2024_04_26_145907AD09, $Kernel, $SD, psxsweepBPfilter(300,2,2), 5, psxRiseTime(),psxDeconvBPFilter())

t-b assigned timjarsky Feb 28, 2025

t-b requested review from MichaelHuth and timjarsky as code owners February 28, 2025 17:55

t-b enabled auto-merge February 28, 2025 17:59

This comment was marked as outdated.

Sign in to view

timjarsky assigned t-b and unassigned timjarsky Mar 7, 2025

This comment was marked as outdated.

Sign in to view

t-b assigned timjarsky and unassigned t-b Mar 11, 2025

t-b assigned t-b and unassigned timjarsky Mar 18, 2025

t-b force-pushed the bugfix/2362-psx-support-multiple-selections branch from f508957 to f23db8a Compare March 19, 2025 14:41

t-b assigned timjarsky and t-b and unassigned t-b and timjarsky Mar 19, 2025

t-b mentioned this pull request Mar 25, 2025

PSX multiple selections (only that) #2376

Merged

t-b changed the title ~~Support multiple select statements~~ Various PSX fixes Mar 25, 2025

t-b force-pushed the bugfix/2362-psx-support-multiple-selections branch from 1332271 to 08deb18 Compare March 26, 2025 18:12

t-b assigned timjarsky and unassigned timjarsky Mar 26, 2025

t-b force-pushed the bugfix/2362-psx-support-multiple-selections branch 4 times, most recently from a12c1dd to 193bf80 Compare April 8, 2025 19:46

This comment was marked as outdated.

Sign in to view

t-b and others added 13 commits October 29, 2025 22:48

MIES_Utilities_Algorithm.ipf: Add helper routines for DoFFT

2387ca4

These routines allow to calculate all values between 2^1 and 2^30 which have 2 and 3 as prime factors. This helps in deciding a good cut off value for FFT as it only performs when the primes are smaller than 5.

PSX: Add Onset and Rise y values to psxEvent

282b111

And allow querying them from psxstats.

PSX_FitAverage: Store fit results in datafolder hierarchy

5e04066

Packages/doc/SweepFormula.rst: Add units for rise and decay tau

db96e98

PSX_Operation: Add peakThreshold to parameter JSON

5a6bdcf

This is convenient for documentation purposes although this is not an input parameter.

PSX_OperationStatsImpl: Output more entries for stats postprocessing

a651366

Tim's questionable changes :)

79445d3

PSX_UpdateAverageTraces: Fix indexing confusion

4cbb3a8

Ever since 7aa2be6 (PSX: Add average fit for all states, 2025-06-04) we are using the per-combo index for gathering the eventKernelAmp/eventOnsetTime/eventPeakTime values. This is wrong as we must use the index of the displayed events.

new function to determine the fit end time for the decay to the avera…

53d99a0

…ge waveform

Update Packages/MIES/MIES_SweepFormula_PSX.ipf

d9ddd44

camel case fix Co-authored-by: Copilot <[email protected]>

Update Packages/MIES/MIES_SweepFormula_PSX.ipf

ba0dc32

camel case fix Co-authored-by: Copilot <[email protected]>

t-b force-pushed the bugfix/2362-psx-support-multiple-selections branch from a497d56 to ba0dc32 Compare October 29, 2025 22:14

t-b assigned timjarsky and t-b and unassigned t-b and timjarsky Dec 3, 2025

fixup! psxSweepBPFilter: Add it

9ffd16b

Copilot AI review requested due to automatic review settings December 10, 2025 14:10

t-b assigned timjarsky and unassigned t-b Dec 10, 2025

Copilot started reviewing on behalf of t-b December 10, 2025 14:11 View session

Copilot AI reviewed Dec 10, 2025

View reviewed changes

timjarsky assigned t-b and unassigned timjarsky Dec 16, 2025

		duplicate/O AvgEventDiff, root:forDisp
		smooth 500, root:forDisp

	// 5 NaNs for the first event only, the rest is onset Time
	// For the first event, 5 fields are expected to be NaN: weightedTau, slowTau, fastTau, onsetY, and riseY.
	// This is because these fields are not defined for the first event due to lack of preceding data or fit results.

	`7`, for the frequencies `500` (`lowFreq`) and `50` (`highFreq`).
	`4`. There are no default frequencies defined in the implementation.

	variable level = (riseLowerThreshold * (extrema - average(onsetX))) + average(onsetX)
	variable level = (riselowerThreshold * (extrema - average(onsetX))) + average(onsetX)

Various PSX fixes #2362

Are you sure you want to change the base?

Various PSX fixes #2362

Uh oh!

Conversation

t-b commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

timjarsky commented Apr 8, 2025

Uh oh!

timjarsky commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timjarsky commented Dec 3, 2025

Uh oh!

t-b commented Dec 3, 2025

Uh oh!

t-b commented Dec 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

timjarsky commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

t-b commented Feb 28, 2025 •

edited

Loading

timjarsky commented Apr 8, 2025 •

edited

Loading