cosanlab / nltools

Python toolbox for analyzing imaging data
https://nltools.org
MIT License
120 stars 42 forks source link

consider making Design_Matrix more flexible with custom index #347

Open maxfarrens opened 4 years ago

maxfarrens commented 4 years ago
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 commented 4 years ago
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?

ejolly commented 4 years ago

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.

ljchang commented 4 years ago

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.

ejolly commented 4 years ago

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.