cliffordlab / PhysioNet-Cardiovascular-Signal-Toolbox

PhysioNet Cardiovascular Signal Toolbox
BSD 3-Clause "New" or "Revised" License
119 stars 57 forks source link

RRI time and RRI index used in same variable in prsa.m #14

Closed embar- closed 5 years ago

embar- commented 5 years ago

Second input variable for prsa.m function is "rri" is ambiguous and is used in two different contexts: index and time.

Cases where "rri" seems to be used as index: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L8 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L92


But "rri" is also re-used in context of time: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L60


Also "rri" comes from Main_HRV_Analysis.m (as "tNN"): https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L170 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L140 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L292 "tNN" created in RRIntervalPreprocess: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L297 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/Preprocessing/RRIntervalPreprocess.m#L1 according to "cleantNN" description, it "tNN" (and then "rri") should be index: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L297 Though computationally it could be time variable in seconds also. Anyway, source is "t" variable at "Main_HRV_Analysis.m", that also has ambiguous definition: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L13


If these variables are time variables (units are seconds), then there is contradiction with cases where it is used as index.

If it is index please remove unit name "seconds" from Main_HRV_Analysis.m and include check whether it is integer. Also seems, we usually don't need it at all as input, because we can generate it internally, e.g. for Main_HRV_Analysis.m

if strcmpi(InputFormat,'RRIntervals')
   t=1:length(InputSig)
end

in other cases it described not be used anyway: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L14

Or to generate internally for prsa.m: rri=1:length(rr)


Please clarify these variables.

GiuliaDAP commented 5 years ago

Thanks for this comment. rri, t and tNN are time indices, indicating the time [seconds] related to a RR or NN interval. I’ll make sure the naming is consistent in all the functions and update description accordingly.

https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L92 nn_win = rr( rri >= WinStarIdxs(i_win) & rri < WinStarIdxs(i_win) + windowlength ); rri is used to generate indices not as an index

On May 16, 2019, at 7:10 AM, embar- notifications@github.com wrote:

Second input variable for prsa.m function is "rri" is ambiguous and is used in two different contexts: index and time.

Cases where "rri" seems to be used as index: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L8 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L92

But "rri" is also re-used in context of time: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/PRSA_Tools/prsa.m#L60

Also "rri" comes from Main_HRV_Analysis.m (as "tNN"): https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L170 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L140 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L292 "tNN" created in RRIntervalPreprocess: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L297 https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Tools/Preprocessing/RRIntervalPreprocess.m#L1 according to "cleantNN" description, it "tNN" (and then "rri") should be index: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L297 Though computationally it could be time variable in seconds also. Anyway, source is "t" variable at "Main_HRV_Analysis.m", that also has ambiguous definition: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L13

If these variables are time variables (units are seconds), then there is contradiction with cases where it is used as index.

If it is index please remove unit name "seconds" from Main_HRV_Analysis.m and include check whether it is integer. Also seems, we usually don't need it at all as input, because we can generate it internally, e.g. for Main_HRV_Analysis.m

if strcmpi(InputFormat,'RRIntervals') t=1:length(InputSig) end

in other cases it described not be used anyway: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/aebdcc0d294158d6de1bff2088ca6e276d6ca97d/Main_HRV_Analysis.m#L14

Or to generate internally for prsa.m: rri=1:length(rr)

Please clarify these variables.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

embar- commented 5 years ago

nn_win = rr( rri >= WinStarIdxs(i_win) & rri < WinStarIdxs(i_win) + windowlength ); rri is used to generate indices not as an index

Yes, but rri is compared to WinStarIdxs, which ends with ...Idxs – name suggesting to index (of of time point), not to time (in seconds).

embar- commented 5 years ago

Thanks for this comment. rri, t and tNN are time indices, indicating the time [seconds] related to a RR or NN interval.

Suggesting descriptions with units: "time (in seconds)" or "indices (integers) of time points". Please don't mention word "seconds" near "time indices" or "time points".

embar- commented 5 years ago

Error message could also be enhanced: https://github.com/cliffordlab/PhysioNet-Cardiovascular-Signal-Toolbox/blob/0a3820068b5ab07345e7fbc5509174832a914f0a/Main_HRV_Analysis.m#L118:L121