Conversation
dominikbach
added
Completed & Waiting for Review
dominikbach
added
In Progress
dominikbach
added
Completed & Waiting for Review
teddphil
added
Under Review
and removed
Completed & Waiting for Review
teddphil
left a comment
teddphil
left a comment
There was a problem hiding this comment.
Just made some minor suggestions. The updates to the functionality do not have issues to me. Thank you.
| % ● Arguments | ||
| % indata: [struct/char/numeric] or [cell array of struct/char/numeric] | ||
| % contains the data to be interpolated | ||
| % indata: [char/numeric] contains the data to be interpolated |
There was a problem hiding this comment.
Think the description of indata shall be updated to make it inline with above for avoiding confusion...
|
|
||
| if isnumeric(indata) | ||
| if nargin >= 2 | ||
| options = varargin{1}; |
There was a problem hiding this comment.
There is no issue for this situation, but I have a feeling that this refers to indata=numeric_array and channel does not exist in this case, so nargin can be only 1 or 2. ">2" is a situation that is unexpected.
|
|
||
| %% 2 work on data | ||
| % 2.1 load data | ||
| if method == 1 |
There was a problem hiding this comment.
I believe this method is different from options.method that is explained in help text. Maybe it is useful to explain the meaning of method 1, 2, 3 for future maintaining purposes.
| % indata: [struct/char/numeric] or [cell array of struct/char/numeric] | ||
| % contains the data to be interpolated | ||
| % indata: [char/numeric] contains the data to be interpolated | ||
| % channel: a single channel identifier accepted by pspm_load_channel |
There was a problem hiding this comment.
I think accepting only one channel will be better because (1) it is simpler and can avoid bugs better (2) users can simply repeating this function in their script if they want to process multiple channels.
| end | ||
| if s_overlap || e_overlap | ||
| if ~options.extrapolate | ||
| warning('ID:option_disabled', ... |
There was a problem hiding this comment.
I am not sure I understand option_disabled, does this mean options.extrapolate must be true? It seems to be option_invalid which can be in line with input_invalid.
There was a problem hiding this comment.
I suggest to add some help text to cm. For example, it can just let users know "only one channel will be processed under channel mode".
There was a problem hiding this comment.
And the channel action is renamed to mode here. I do not have a preference, but I think consistency can make users feel less confused about what they mean.
| 'previous', ... | ||
| 'next'} ); | ||
|
|
||
| options = autofill(options, 'newfile', 0, 1 ); |
There was a problem hiding this comment.
If this newfile is no longer used, I suggest to remove this line.
teddphil
added
Approved
teddphil
removed
the
Approved
2 participants
Fixes #673 : refactors
pspm_interpolate.Changes proposed in this pull request:
pspm_load_channelfor single channel option, andpspm_select_channelsfor all channel option.pspm_load_datagracefully handles this, but producing the output is more complex.)manytoany. Also, file selector is changed to allowing just one file at a time.options.extrapolatewas forced to1. However, then the option does not make sense, as it would always be enforced whenever it is relevant. Thus, this is no longer enforced, but a warning is still given that NaN remain in the data.pspm_optionsis changed to deal with the larger number of methods allowed.pspm_load_channelis changed to allow detecting whether a channel is a wave or events channel.