Unify function output #689

dominikbach · 2024-04-17T18:54:05Z

Fixes #624. Fixes #519. Fixes #421.

Changes proposed in this pull request:

Functions that write data into channels always return [sts, channel_index] and potentially further output arguments.
Functions that create new files always return [sts, newfilename] and potentially further output arguments.
Functions always work on one data file only unless explicitly designed to work on multiple files (such as pspm_exp).
pspm_write_channel is always called with an index of the channels that were used to create the new channel data. pspm_write_channel will then check whether these channels will be overwritten (if action is replace and the channels have the same type and units) or whether new channels are created.
varargout was removed wherever possible. Some widely used helper functions remain (e.g. pspm_overwrite and pspm_prepdata) and will be addressed in new pull requests.
GUIs and tests were changed accordingly.
Some inconsistencies remain in cfg_run functions and will be addressed in new PR.

Summaries:

Function output was changed:

pspm_combine_markerchannels
pos_of_channel
pspm_convert_au2unit
pspm_convert_ecg2hb
pspm_convert_hb2hp
pspm_convert_ppg2hb
pspm_emg_pp
pspm_find_sounds
pspm_resp_pp
pspm_scr_pp
pspm_pp
pspm_ren
pspm_remove_epochs
pspm_import
pspm_trim
pspm_glm
pspm_interpolate

Write channel behaviour changed

pspm_convert_au2unit
pspm_convert_gaze
pspm_pupil_correct_eyelink
pspm_pupil_pp
pspm_scr_pp
pspm_remove_epochs

Option to work on multiple files removed

pspm_scr_pp
pspm_merge
pspm_trim
pspm_import

Varargout removed

pspm_merge
pspm_split_sessions
pspm_ren
pspm_trim
pspm_dcm
pspm_glm_recon
pspm_sf
pspm_sf_auc
pspm_sf_dcm
pspm_sf_mp
pspm_sf_scl
pspm_transfer_function
pspm_version
pspm_write_channel

Function output unchanged

pspm_convert_ecg2hb_amri
pspm_pupil_correct_eyelink
pspm_pupil_pp

Bugs fixed

pspm_load_channel

Other changes

pspm_ren was renamed to pspm_rename for enhanced transparency
pspm_rename now uses pspm_load_data to save data and checks whether new file already exists, via options.overwrite
improved pspm_write_channel, now uses pspm_select_channels for checks
new function pspm_check_data, used in pspm_load_data and pspm_write_channel
improved pspm_select_channels

teddychao · 2024-04-19T13:40:41Z

Hi Dom, Thank you for the update. My flight is tomorrow and I am preparing for several stuffs. Is it ok if I review this next week? Best wishes Teddy From: Dominik Bach ***@***.***> Date: Friday, 19 April 2024 at 09:29 To: bachlab/PsPM ***@***.***> Cc: Teddy ***@***.***>, Review requested ***@***.***> Subject: Re: [bachlab/PsPM] Unify function output (PR #689) @dominikbach<https://github.com/dominikbach> requested your review on: #689<#689> Unify function output. — Reply to this email directly, view it on GitHub<#689 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA5W62TSH2LVJPQBERKYIDLY6DIX5AVCNFSM6AAAAABGL33LHKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGUZTINZWHA2DMMQ>. You are receiving this because your review was requested.Message ID: ***@***.***>

teddychao

I have reviewed some functions.. I try to finish them in these two days.

src/pspm_check_data.m

src/pspm_cfg/pspm_cfg_merge.m

src/pspm_cfg/pspm_cfg_rename.m

teddychao · 2024-05-13T12:14:56Z

src/pspm_cfg/pspm_cfg_run_find_sounds.m

The Line 24--27 may not work as fully expected, because the channel action does not have a fallback. Users cannot avoid creating a channel. To fix this, I suggest to change line 24--27 to be

if isfield(d.create_corrected_chan, 'yes') options.channel_action = 'add'; options.channel_output = 'corrected'; else options.channel_action = 'none'; end

And in pspm_options, it should be modified in line 217, to be

options = autofill_channel_action(options, 'add', {'replace', 'none'} );

The new design is to always create a channel (i.e. never give the GUI user the option not to). But this was incorrectly implemented in the GUI. It should be fixed now.

src/pspm_sf_mp.m

src/pspm_pupil_correct_eyelink.m

teddychao · 2024-05-13T13:39:13Z

src/pspm_merge.m

+infos = cell(2,1);
+data = cell(2,1);
+for iNum = 1:2
+    [sts_load_data, infos, data] = pspm_load_data(infile{iNum});


This line does not seem to be right. I run the data with the GUI's merge feature, and the variable fn cannot be identified correctly. This is because the input is a cell instead of a string.

Thanks for spotting this; pspm_cfg_run_merge is fixed now.

teddychao · 2024-05-13T14:02:08Z

src/pspm_cfg/pspm_cfg_run_rename.m

I am not sure this file is working. I checked and saw it reported error when I tried to rename a file. The job.file did not exist in this occasion. My understanding of what has been updated to pspm_cfg_run_merge is to make it consistent to the output from other functions, so it is not expecting to change the functionality of "rename". Maybe you could have a look and see if any unexpected update has caused this issue?

Thank you, this is fixed now.

teddychao · 2024-05-13T14:05:15Z

src/pspm_combine_markerchannels.m

@@ -1,26 +1,29 @@
-function sts = pspm_combine_markerchannels(datafile, options)
+function [sts, outchannel] = pspm_combine_markerchannels(datafile, options)


It may be useful to update pspm_cfg_run_combine_markerchannels and add output at its line 11. Similarly, for pspm_combine_markerchannels_test, the lines 22, 28, 48, 66 may need to be updated to add the output.

GUI dependencies & tests will be updated in future PR, no change required for now.

teddychao · 2024-05-13T14:06:24Z

src/pspm_cfg/pspm_cfg_run_pp_emg_data.m

 if sts == 1
-  out = {output.channel};
+  out = {outchannel};


I saw other functions, such as pspm_cfg_run_merge has removed out={out} if it is not a cell. I am not sure if this is still necessary here?

GUI dependencies will be updated in a future PR.

teddychao · 2024-05-13T15:26:13Z

test/pspm_scr_pp_test.m

@@ -82,30 +75,16 @@ function scr_pp_test_missing(this, channels)
        'deflection_threshold', 0, ...
        'expand_epochs', 0, ...
        'channel_action', 'add');
-      options6 = struct('change_data', 0);
      % Verifying the situation with missing epochs filename option without
      % saving to datafile
      pspm_testdata_gen(channels, this.duration, this.fn);
      [~, out] = pspm_scr_pp(this.fn, options4);


I think for the test here, options4 shall include the channel_action, in this case shall be set to be none or withdraw. If not set, this channel_action will use the default fallback, which is add. It is better to sort it to be none which means the processed data will not be saved to the original file.

Tests can be updated later.

teddychao · 2024-05-13T17:03:36Z

src/pspm_convert_ecg2hb.m

 % ● Description
 %   pspm_convert_ecg2hb identifies the position of QRS complexes in ECG data and
 %   writes them as heart beat channel into the datafile. This function
 %   implements the algorithm by Pan & Tompkins (1985) with some adjustments.
 % ● Format
-%   sts = pspm_convert_ecg2hb(fn, options)
+%   [sts, channel_index, quality_info] = pspm_convert_ecg2hb(fn, options)


maybe it is useful to say outchannel and debug_info here, since they are named in the code..

Stylistic issue, no change required.

teddychao · 2024-05-13T17:08:49Z

src/pspm_convert_ecg2hb_amri.m

@@ -9,8 +9,8 @@
 %   it will either replace an existing heartbeat channel or add it as a new
 %   channel to the provided file.
 % ● Format
-%   [sts, out_channel] = pspm_convert_ecg2hb_amri(fn)
-%   [sts, out_channel] = pspm_convert_ecg2hb_amri(fn, options)
+%   [sts, channel_index] = pspm_convert_ecg2hb_amri(fn)


Similarly, both channel_index and out_channel can be used, but better to choose one of them and rename the variables to it...

Just stylistic.

src/pspm_convert_au2unit.m

src/pspm_convert_hb2hp.m

src/pspm_convert_ppg2hb.m

teddychao · 2024-05-13T17:32:40Z

test/pspm_convert_gaze_test.m

-      this.fn = pspm_import(this.raw_input_filename, 'eyelink', import, options);
-      this.fn = this.fn{1};
+      [sts, this.fn] = pspm_import(this.raw_input_filename, 'eyelink', import, options);
+      this.fn = this.fn;


Think this line can be just deleted...

teddychao · 2024-05-13T19:06:41Z

src/pspm_cfg/pspm_cfg_run_pp_heart_data.m

@@ -23,20 +23,20 @@
                                     {'semi', 'twthresh'});
        options = pspm_update_struct(options, job, 'channel_action');
        % call function
-        [sts, winfo] = pspm_convert_ecg2hb(fn, options);
+        [sts, outchannel] = pspm_convert_ecg2hb(fn, options);
      case 'ecg2hb_amri'
        options = pp_field.opt;
        options.channel = chan;
        options = pspm_update_struct(options, job, 'channel_action');
        winfo = struct();


Think this line can be removed.

teddychao · 2024-05-13T19:13:49Z

src/pspm_find_sounds.m

 % ● Description
 %   pspm_find_sounds finds and if required analyzes sound events in a pspm file.
 %   A sound is accepted as event if it is longer than 10 ms and events are
 %   recognized as different if they are at least 50 ms appart.
 % ● Format
-%   [sts, infos] = pspm_find_sounds(file,options)
+%   [sts, channel_index, info] = pspm_find_sounds(fn, options)


Maybe consistent variable naming is required here too

Just stylistic.

src/pspm_find_sounds.m

…unify-function-output

Dominik Bach added 20 commits April 12, 2024 14:53

modify pspm_combine_markerchannels

5e40e47

modify pspm_convert_au2unit and pspm_convert_area2diameter

a803376

fix pspm_load_channel

9b391cc

modify pspm_convert_hb2hp

cb6813c

modify pspm_convert_ppg2hb & pspm_emg_pp

9e5d630

modify pspm_find_sounds

2608a84

modify pspm_pupil_correct_eyelink

3ecf795

modify pspm_resp_pp

25345c1

modify pspm_scr_pp

545af94

modify pspm_pp

d69277b

modify pspm_merge

5e973c7

modify pspm_split_sessions

48a9edb

modify pspm_remove_epochs

9d62414

modify pspm_trim

8b092c1

modify pspm_import

afc20b9

modify pspm_dcm

ffe2f70

modify pspm_dcm_inv

ae4a435

modify pspm_glm

ba1d6ff

remove varargout in pspm_sf_... and pspm_transfer_function

91d3fca

modify pspm_version

72eaf15

dominikbach added the In Progress Currently being worked on label Apr 17, 2024

Dominik Bach and others added 8 commits April 18, 2024 11:09

fixes after testing

06bad35

fixes after testing

cb29ec1

Merge branch 'develop' into unify-function-output

31618c4

Merge branch 'develop' into unify-function-output

f2225b9

modify pspm_interpolate

c88cca3

improve pspm_select_channels

94cfdc8

test fix

109035e

test fix

6b9c365

dominikbach requested a review from teddychao April 19, 2024 08:29

dominikbach self-assigned this Apr 19, 2024

dominikbach added Completed & Waiting for Review Completed and waiting for review and removed In Progress Currently being worked on labels Apr 19, 2024

dominikbach mentioned this pull request Apr 20, 2024

Fix markerinfo handling #695

Open

dominikbach mentioned this pull request May 6, 2024

Helper function to process multiple channels at a time #705

Merged

teddychao requested changes May 13, 2024

View reviewed changes

teddychao reviewed May 13, 2024

View reviewed changes

teddychao requested changes May 13, 2024

View reviewed changes

Dominik Bach and others added 6 commits May 14, 2024 16:08

remove comment numbering in pspm_check_data

2dbd2fb

fix pspm_cfg_rename

4969a2b

fix pspm_cfg_find_sounds

ef8241b

Merge branch 'develop' into unify-function-output

b7fdc3e

fix pspm_version and GUI for pspm_merge

5a8a12b

fix pspm_rename and GUI item

bd999ac

teddychao approved these changes May 21, 2024

View reviewed changes

Dominik Bach added 3 commits May 21, 2024 19:03

improve test for gaze_convert

9f8a9b4

Merge branch 'unify-function-output' of github.com:bachlab/PsPM into …

74ceee8

…unify-function-output

fix test for pspm_rename

17e42f1

dominikbach merged commit 7ccbf06 into develop May 21, 2024
1 check passed

dominikbach deleted the unify-function-output branch May 21, 2024 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify function output #689

Unify function output #689

dominikbach commented Apr 17, 2024 •

edited

teddychao commented Apr 19, 2024 via email

teddychao left a comment

teddychao May 13, 2024

dominikbach May 15, 2024 •

edited

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 15, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 21, 2024

teddychao May 13, 2024

dominikbach May 15, 2024

		@@ -1,26 +1,29 @@
		function sts = pspm_combine_markerchannels(datafile, options)
		function [sts, outchannel] = pspm_combine_markerchannels(datafile, options)

Unify function output #689

Unify function output #689

Conversation

dominikbach commented Apr 17, 2024 • edited

Changes proposed in this pull request:

Summaries:

Function output was changed:

Write channel behaviour changed

Option to work on multiple files removed

Varargout removed

Function output unchanged

Bugs fixed

Other changes

teddychao commented Apr 19, 2024 via email

teddychao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikbach May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikbach commented Apr 17, 2024 •

edited

dominikbach May 15, 2024 •

edited