Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Step parameter not used during RSASA construction #282

Open
jchutrue opened this issue Mar 16, 2019 · 6 comments
Open

Step parameter not used during RSASA construction #282

jchutrue opened this issue Mar 16, 2019 · 6 comments

Comments

@jchutrue
Copy link
Member

If we instantiate an RSASA without specifying the abscissa values, only fs is used to infer them:
x = nel.AnalogSignalArray(np.random.rand(5, 10), fs=2).

This works, x.abscissa_vals returns array([ 0. , 0.5, 1. , 1.5, 2. , 2.5, 3. , 3.5, 4. , 4.5]).

The equivalent passing in step as an argument is y = nel.AnalogSignalArray(np.random.rand(5, 10), step=0.5). However, this does not work as expected; it's abscissa values are array([ 0., 1., 2., 3., 4., 5., 6., 7., 8., 9.]).

My own opinion is to remove step in the constructor and infer it from the inverse of fs.

@eackermann
Copy link
Member

If memory serves, step was meant for weird cases where you know you have missing data, or screwy data, for which fs wouldn't be computed accurately anyway.

E.g., Trodes data is decimated by taking only every 10th sample, so abscissa_vals = [1, 11, 21, 31, 51, 61] is passed in. Then, fs cannot be computed from the data, and moreover, get_contiguous_events may want to say that [1] , [11], [21], ... are all individual epochs. Instead, with step=10, we would get [1, 31], and [51, 61] as two contiguous epochs.

@eackermann
Copy link
Member

We can definitely handle the inputs a bit better, but step and fs were never meant to be alternatives for the same purpose. In the Trodes example, we may still want to specify fs=30,000 with some step, even though we'll have way fewer than the expected number of samples. I'd never recommend such an approach, but it could be used in the process of getting to "good, regular, well-defined" data.

@eackermann
Copy link
Member

I'd go as far as to say that step should maybe only be allowed when abscissa_vals are given.

@jchutrue
Copy link
Member Author

OK I'm just trying to understand this a little better. If abscissa_vals are [1, 11, 21, 31, 51, 61] and you wanted to convert them into seconds, why would dividing them by the decimated sampling rate not work?

@eackermann
Copy link
Member

That's a good question. And to be honest, I don't know, or I don't remember. This is a legacy part from back when .time and .timestamps were two, separate things! so you could pass in timestamps, or pass in sample numbers, for which a step argument might have made sense.

As of right now, I can't think of a case where passing in step would be superior to passing in a sampling rate, if known.

@shayokdutta do you still have a use-case in mind for step? I'll have to think very carefully, too, in case I'm missing something right now.

@jchutrue
Copy link
Member Author

Right, because currently we expect the abscissa values to be in units of time, specifically seconds. We may support other units in the future, though. Generalizing the interpretation of abscissa values would be useful for a nelpy 2.0 discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants