Extend apfrequency operation with method instantaneous pair #1607

MichaelHuth · 2022-12-27T18:05:28Z

The new method number for apfrequency is 3.

MichaelHuth · 2022-12-27T18:08:10Z

@timjarsky Before I add tests and docu, could you please verify that the calculation works as intended by the brief issue?
I added kHz as label is the input wave units are ms, though it's quite a stretch to define a frequency from two data points.

timjarsky · 2023-01-04T19:15:13Z

@MichaelHuth The new method is looking good. Can you add the unit to the y-axis? I also need help understanding the x-axis unit. It should reflect the AP number in some way.

MichaelHuth · 2023-01-09T13:21:18Z

@timjarsky I think the kHz should actually be on the y-axis. (I will fix this). About the x-axis in the issue is stated

and then user wants to plot spikes vs x values
The x values are the time of the left spike of the pair in ms.
Does that make sense? From your comment it sound like x should just count the (interleaved) pairs.

timjarsky · 2023-01-09T17:17:44Z

Hi @MichaelHuth,

Let's keep the x-axis as spike time but add an option to have the x-axis be spike count.

I'd also like to add an option to have the Y values be time or frequency (default to frequency).

One final option is to have the Y-values be normalized to the first interval time/frequency (default to non-normalized).

Thank you!

t-b · 2023-01-09T20:45:28Z

@MichaelHuth It might make sense to have these new optional parameters for all methods.

MichaelHuth · 2023-01-11T23:30:25Z

@timjarsky So the third option would actually be two options?

divides all results by the first spike pair interval time
divide all results by the first pair frequency

timjarsky · 2023-01-11T23:33:10Z

Yes, you can think of it that way. I was first defining whether the user wants to work with frequency or time, then letting the user decide if they want the data normalized or not.

MichaelHuth · 2023-01-23T17:42:32Z

I added two more optional parameters:
time/freq~ and normalize/nonormalize~
~ = default

time/freq has only an effect for the result values on method 3 (pairs) as I currently don't see a senseful application on the other methods that return integrated values.

normalize works on all methods and requires at least 2 peaks per sweep to be applied. The normalization divisor depends on the time/freq setting.

I have fixed also the Y-axis label and simplified the X-axis label under the assumption that the input is always sweep data with ms a x wave units.

timjarsky · 2023-01-23T22:19:06Z

@MichaelHuth, thank you for adding the new features. I would still like to add an x-axis option that plots the frequency/time (Y) values against spike count instead of spike time.

Minor: when using the normalized option, y-axis labels could be something like: "normalized frequency" instead of (Hz), which is confusing.

timjarsky · 2023-01-24T18:46:32Z

@MichaelHuth In addition to axis label changes, you may also want to utilize the legend labels to make plots like this interpretable.

timjarsky · 2023-01-24T18:52:14Z

@MichaelHuth It would also be helpful if the Y-axis label included the method string.

MichaelHuth · 2023-02-03T19:48:40Z

Good for another try. I added after normalize another argument that can either be time or count. With time as default.

The bigger part was to make traces and annotations from different formulas in the same plot distinguishable.
I added that as general approach but it is currently setup only for apfrequency.
So with your example from the image it should now adapt the annotation and show what argument is different and change the marker/line style. For the same arguments the style in one plot stays the same for multiple formulas combined with the "with" keyword.

timjarsky · 2023-02-08T17:01:58Z

@MichaelHuth, how are the normalized values calculated with method full?

I was expecting the value to be normalized to the first sweep.

(third from the top plot)

MichaelHuth · 2023-02-09T19:13:42Z

The normalization factor does not depend on the method, only if you use time or freq.
For formula three you use freq, so

peakTimeFreq = 1.0 / ( (peaksAt[1] - peaksAt[0]) * MILLI_TO_ONE) // frequency in Hz from the first two peaks
result = result / peakTimeFreq // apply normalization factor

timjarsky · 2023-02-13T17:34:36Z

The normalization factor does not depend on the method, only if you use time or freq. For formula three you use freq, so
peakTimeFreq = 1.0 / ( (peaksAt[1] - peaksAt[0]) * MILLI_TO_ONE) // frequency in Hz from the first two peaks
result = result / peakTimeFreq // apply normalization factor

Thanks for the explanation @MichaelHuth So, I think normalization should depend on the method. For methods where a single value is returned for each sweep, the normalization should be across all sweeps.

timjarsky · 2023-02-20T17:23:00Z

@MichaelHuth , i want to modify what I said above. Rather than have the normalization depend on the method, i think it makes sense to give the user to specify if they want the normalization to occur within the sweep or across sweeps.

t-b · 2023-03-22T16:35:23Z

Failing tests:

Expand | Failed | PatchSeqAccessResistanceSmoke#PS_AR2:NI                                 PatchSeqAccessResistanceSmoke#PS_AR2:NI in UTF_PatchSeqAccessResistanceSmoke.ipf (48)                                 History | Failing since build #3                                     (Code changes detected) | 12 secs
-- | -- | -- | -- | --
Expand | Failed | PatchSeqAccessResistanceSmoke#PS_AR3:NI                                 PatchSeqAccessResistanceSmoke#PS_AR3:NI in UTF_PatchSeqAccessResistanceSmoke.ipf (51)                                 History | Failing since build #3                                     (Code changes detected) | 20 secs
Expand | Failed | PatchSeqAccessResistanceSmoke#PS_AR4:NI                                 PatchSeqAccessResistanceSmoke#PS_AR4:NI in UTF_PatchSeqAccessResistanceSmoke.ipf (54)                                 History | Failing since build #3                                     (Code changes detected) | 22 secs
Expand | Failed | PatchSeqAccessResistanceSmoke#PS_AR6:NI                                 PatchSeqAccessResistanceSmoke#PS_AR6:NI in UTF_PatchSeqAccessResistanceSmoke.ipf (60)                                 History | Failing since build #3                                     (Code changes detected) | 20 secs
Expand | Failed | PatchSeqAccessResistanceSmoke#PS_AR6a:NI                                 PatchSeqAccessResistanceSmoke#PS_AR6a:NI in UTF_PatchSeqAccessResistanceSmoke.ipf (63)                                 History | Failing since build #3                                     (Code changes detected) | 20 secs
Expand | Failed | PatchSeqAccessResistanceSmoke#PS_AR8:NI                                 PatchSeqAccessResistanceSmoke#PS_AR8:NI in UTF_PatchSeqAccessResistanceSmoke.ipf (69)                                 History | Failing since build #3                                     (Code changes detected) | 13 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE1:NI                                 PatchSeqSealEvaluation#PS_SE1:NI in UTF_PatchSeqSealEvaluation.ipf (198)                                 History | Failing since build #3                                     (Code changes detected) | 18 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE2:NI                                 PatchSeqSealEvaluation#PS_SE2:NI in UTF_PatchSeqSealEvaluation.ipf (201)                                 History | Failing since build #3                                     (Code changes detected) | 12 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE5:NI                                 PatchSeqSealEvaluation#PS_SE5:NI in UTF_PatchSeqSealEvaluation.ipf (210)                                 History | Failing since build #3                                     (Code changes detected) | 18 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE6:NI                                 PatchSeqSealEvaluation#PS_SE6:NI in UTF_PatchSeqSealEvaluation.ipf (213)                                 History | Failing since build #3                                     (Code changes detected) | 18 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE7:NI                                 PatchSeqSealEvaluation#PS_SE7:NI in UTF_PatchSeqSealEvaluation.ipf (216)                                 History | Failing since build #3                                     (Code changes detected) | 11 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE7a:NI                                 PatchSeqSealEvaluation#PS_SE7a:NI in UTF_PatchSeqSealEvaluation.ipf (219)                                 History | Failing since build #3                                     (Code changes detected) | 18 secs
Expand | Failed | PatchSeqSealEvaluation#PS_SE9:NI                                 PatchSeqSealEvaluation#PS_SE9:NI in UTF_PatchSeqSealEvaluation.ipf (225)                                 History | Failing since build #3                                     (Code changes detected) | 12 secs
Expand | Failed | SweepFormulaHardware#SF_TPTest:NI                                 SweepFormulaHardware#SF_TPTest:NI in UTF_SweepFormulaHardware.ipf (393)                                 History | Failing since build #3                                     (Code changes detected) | 15 secs
Expand | Failed | UTF_SweepFormula#StoreWorks:0                                 UTF_SweepFormula#StoreWorks:0 in UTF_SweepFormula.ipf (1095)                                 History | Failing since build #4                                     (Code changes detected) | < 1 sec
Expand | Failed | UTF_SweepFormula#StoreWorks:1                                 UTF_SweepFormula#StoreWorks:1 in UTF_SweepFormula.ipf (1098)                                 History | Failing since build #4                                     (Code changes detected) | < 1 sec
Expand | Failed | UTF_SweepFormula#StoreWorks:2                                 UTF_SweepFormula#StoreWorks:2 in UTF_SweepFormula.ipf (1101)                                 History | Failing since build #4                                     (Code changes detected) | < 1 sec
Expand | Failed | UTF_SweepFormula#TPWithModelCell                                 UTF_SweepFormula#TPWithModelCell in UTF_SweepFormula.ipf (1209)                                 History | Failing since build #4                                     (Code changes detected) | < 1 sec

t-b · 2023-03-23T17:54:44Z

Review:

Nice test coverage for apfrequency! I've only looked at the code.

Missing comma in parameter list. Igor groks it but our doxygen-filter-ipr does not. It fails without error. I've created Missing comma before optional parameters gives wrong result byte-physics/doxygen-filter-ipf#26 but a quick look at the code suggests that this is not so easy to fix.

+Function/WAVE SFH_GetOutputForExecutorSingle(WAVE/Z data, string graph, string opShort[variable discardOpStack, WAVE clear])

We have now two places in SF_FormulaPlotter were we apply markers. In line 1700ff and 1755ff. That's not nice.
In addition operations providing per point markers (SF_META_MOD_MARKER) should be preferred over per formula, but constant for the whole trace ones. Or?
Would it make sense to complain in SFH_SerializeArgSetup if one of the %key's is empty?
Please define the number of possible arguments only once in SF_OperationApFrequency. Currently you are using 6 and 5 literally.

- This allows to distinguish if another formula is differently parameterized. This information is suitable to decide if traces should be displayed different or give the user information what parameter is different. "operation stack nice" is operation return information in a human readable form that is more than just the operation name.

- previously the markers property was set globally depending on the data of the last formula plotted. Effectively this was not fully correct since the introduction of the "with" keyword that allows plotting different formulas into the same plot.

… same plot - The marker codes and line codes are defined in a helper function.

- normOverSweepsMin: normalizes over all sweeps based on the minimum result value in all sweeps based on the current method - normOverSweepsMax: normalizes over all sweeps based on the maximum result value in all sweeps based on the current method - normOverSweepsAvg: normalizes over all sweeps based on the average result value in all sweeps based on the current method - normInSweepsMin: normalizes each sweep based on the minimum result value in the specific sweep based on the current method - normInSweepsMax: normalizes each sweep based on the maximum result value in the specific sweep based on the current method - normInSweepsAvg: normalizes each sweep based on the average result value in the specific sweep based on the current method - removed special handling of single peak sweeps, single peaks are treated now in methods that require peak pairs as "no result" - sweeps that contain only a single peak are now treated by the apfrequency methods that require pairs of peaks as data that gives no result. This simplifies the code a lot.

- also add section for argSetup stack

t-b · 2023-03-23T22:32:45Z

@MichaelHuth Nice. Thanks!

MichaelHuth added the SweepFormula label Dec 27, 2022

MichaelHuth requested review from timjarsky and t-b as code owners December 27, 2022 18:05

MichaelHuth self-assigned this Dec 27, 2022

MichaelHuth assigned timjarsky Dec 27, 2022

MichaelHuth force-pushed the feature/1607_extend_sf_apfrequency branch from c99c414 to 9807db5 Compare December 27, 2022 23:48

timjarsky removed their assignment Jan 4, 2023

MichaelHuth force-pushed the feature/1607_extend_sf_apfrequency branch 2 times, most recently from 58abf0a to 5b97273 Compare January 23, 2023 16:16

MichaelHuth force-pushed the feature/1607_extend_sf_apfrequency branch from 77820a9 to c533b27 Compare February 3, 2023 19:43

MichaelHuth assigned timjarsky and unassigned MichaelHuth Feb 6, 2023

timjarsky assigned MichaelHuth and unassigned timjarsky Feb 13, 2023

MichaelHuth force-pushed the feature/1607_extend_sf_apfrequency branch from b8ab81c to bdb78c9 Compare March 22, 2023 13:31

MichaelHuth force-pushed the feature/1607_extend_sf_apfrequency branch from bdb78c9 to df1b74f Compare March 23, 2023 00:42

MichaelHuth assigned t-b and unassigned MichaelHuth Mar 23, 2023

t-b assigned MichaelHuth and unassigned t-b Mar 23, 2023

MichaelHuth added 9 commits March 23, 2023 22:03

SF: Add different markers or line styles if formulas are different in…

09bc1e9

… same plot - The marker codes and line codes are defined in a helper function.

SF: Use helper function for apfrequency argument parsing

709fc47

SF: Update sweepformula help notebook for adapted apfrequency operation

b50d4ba

SF: Adapt docu for new apfrequency arguments

306966d

- also add section for argSetup stack

SF: Add tests for new updated apfrequency operation

59d8e55

SF: Add test for argSetupStack feature

550e545

MichaelHuth force-pushed the feature/1607_extend_sf_apfrequency branch from df1b74f to 550e545 Compare March 23, 2023 21:44

MichaelHuth assigned t-b and unassigned MichaelHuth Mar 23, 2023

t-b enabled auto-merge March 23, 2023 22:32

t-b assigned timjarsky and unassigned t-b Mar 24, 2023

timjarsky approved these changes Mar 24, 2023

View reviewed changes

t-b merged commit a57633a into main Mar 24, 2023

t-b deleted the feature/1607_extend_sf_apfrequency branch March 24, 2023 18:31

timjarsky assigned t-b and MichaelHuth and unassigned timjarsky Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend apfrequency operation with method instantaneous pair #1607

Extend apfrequency operation with method instantaneous pair #1607

MichaelHuth commented Dec 27, 2022

MichaelHuth commented Dec 27, 2022

timjarsky commented Jan 4, 2023

MichaelHuth commented Jan 9, 2023

timjarsky commented Jan 9, 2023

t-b commented Jan 9, 2023

MichaelHuth commented Jan 11, 2023

timjarsky commented Jan 11, 2023

MichaelHuth commented Jan 23, 2023

timjarsky commented Jan 23, 2023

timjarsky commented Jan 24, 2023 •

edited

Loading

timjarsky commented Jan 24, 2023

MichaelHuth commented Feb 3, 2023

timjarsky commented Feb 8, 2023

MichaelHuth commented Feb 9, 2023

timjarsky commented Feb 13, 2023

timjarsky commented Feb 20, 2023

t-b commented Mar 22, 2023

t-b commented Mar 23, 2023 •

edited by MichaelHuth

Loading

t-b commented Mar 23, 2023

Extend apfrequency operation with method instantaneous pair #1607

Extend apfrequency operation with method instantaneous pair #1607

Conversation

MichaelHuth commented Dec 27, 2022

MichaelHuth commented Dec 27, 2022

timjarsky commented Jan 4, 2023

MichaelHuth commented Jan 9, 2023

timjarsky commented Jan 9, 2023

t-b commented Jan 9, 2023

MichaelHuth commented Jan 11, 2023

timjarsky commented Jan 11, 2023

MichaelHuth commented Jan 23, 2023

timjarsky commented Jan 23, 2023

timjarsky commented Jan 24, 2023 • edited Loading

timjarsky commented Jan 24, 2023

MichaelHuth commented Feb 3, 2023

timjarsky commented Feb 8, 2023

MichaelHuth commented Feb 9, 2023

timjarsky commented Feb 13, 2023

timjarsky commented Feb 20, 2023

t-b commented Mar 22, 2023

t-b commented Mar 23, 2023 • edited by MichaelHuth Loading

t-b commented Mar 23, 2023

timjarsky commented Jan 24, 2023 •

edited

Loading

t-b commented Mar 23, 2023 •

edited by MichaelHuth

Loading