MPBA / pyphysio

GNU General Public License v3.0
48 stars 13 forks source link

to_evenly() assigns wrong instants #22

Closed andbiz closed 7 years ago

andbiz commented 7 years ago
signal_unevenly = ph.UnevenlySignal(np.array([1,2,3,4,5,6,8,9,10]),
                                      sampling_freq = 100, signal_nature = '', start_time = 0,
                                      x_values = np.array([1,2,3,4,5,6,7,9,10]),
                                      x_type='instants')
signal_unevenly.get_times()[0] # = 1 <=OK

signal_evenly = signal_unevenly.to_evenly()
signal_evenly.get_times()[0] # = 0 <= KO
Alebat commented 7 years ago

Start time is set to zero so the conversion sets the evenly signal's start time to zero.

Changes: now the start time of the new evenly is set to the first unevenly' sample not caring about the unevenly start time.

Alternatives: pad the signal holding before the first unev. sample till the start time. The absence of signal between start_time and 1st unev sample can maybe be a problem. In this case reopen the issue.

andbiz commented 7 years ago

The bug has reappeared:

import pyphysio as ph
signal_unevenly = ph.UnevenlySignal(np.array([1,2,3,4,5,6,8,9,10]),
                                      sampling_freq = 100, signal_nature = '', start_time = 0,
                                      x_values = np.array([1,2,3,4,5,6,7,9,10]),
                                      x_type='instants')
print(signal_unevenly.get_times()[0]) # = 1 <=OK

signal_evenly = signal_unevenly.to_evenly()
print(signal_evenly.get_times()[0]) # = 0 <= KO
Alebat commented 7 years ago

You set the start_time to be 0:

the difference with test_issues.testissue22 (code posted in the first comment) is that you are setting the start_time=0 instead of None, comparison:

But, it should work in any case, the new evenly should always have as start_time the first unevenly sample as it is not defined before. Should it be NaN instead and start from the original start_time?

I found a commit cf9e676 that changes the start_time of the new signal from the first sample time to the signal's start_time. Who is the author? :smiley: It should be the first time the next start_time, is it correct? I edited :+1:

Also updated issue test to test both None and 0 but it does not work...

Alebat commented 7 years ago

Done