nltools icon indicating copy to clipboard operation
nltools copied to clipboard

consider making Design_Matrix more flexible with custom index

Open maxfarrens opened this issue 5 years ago • 4 comments

Screen Shot 2020-05-04 at 1 20 22 PM

issue appending (adds unwanted rows), using ver 0.3.19

code: `# add motion covariates curr_cov = covs.iloc[time1:time2, :] cov = Design_Matrix(curr_cov, sampling_freq=samp_freq) print(cov.shape)

        # add polynomial regressors
        cov_poly = cov.add_poly(order=2, include_lower=True)
        print(cov_poly.shape)`

output: (45, 49) (53, 52)

maxfarrens avatar May 04 '20 17:05 maxfarrens

Screen Shot 2020-05-04 at 1 27 29 PM

looks like the issue arises when index isn't set at 0. Is this intended behavior for this edge case or should a reset_index be added?

maxfarrens avatar May 04 '20 17:05 maxfarrens

Interesting, yes we assume that the index is numerical and starts from 0 because for most use cases it should. Other than a date-time index (which we don't formally support) it's a bit strange to slice rows and only call Design_Matrix methods on those rows; that was never intended usage. Was there something specific you were trying to accomplish by doing that?

I would recommend a .reset_index to avoid this issue, but I'm going leave this issue open and rename it in case we want to support more flexible use-cases in the future.

ejolly avatar May 04 '20 20:05 ejolly

yeah, we fixed it with .reset_index. I had never seen it before. If we don't do anything, it might be good to note this in the documentation that the indices should be the same length. I'm not sure how I feel about forcing a reset inside the add_poly() method (or any others), we could add a check to make sure indices match, not just length, and return a warning if not.

ljchang avatar May 04 '20 21:05 ljchang

yea I don't love the idea of forcing a reset_index incase the user has a meaningful index they want to retain. Definitely think we should mention it in the docs tho or add a warning if the index doesn't start at 0.

ejolly avatar May 04 '20 21:05 ejolly