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

consider making Design_Matrix more flexible with custom index #347

Open
Tracked by #441
maxfarrens opened this issue May 4, 2020 · 4 comments
Open
Tracked by #441

consider making Design_Matrix more flexible with custom index #347

maxfarrens opened this issue May 4, 2020 · 4 comments

Comments

@maxfarrens
Copy link

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
Copy link
Author

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
Copy link
Collaborator

ejolly commented May 4, 2020

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 ejolly changed the title possible bug with design matrix add_poly consider making Design_Matrix more flexible with custom index May 4, 2020
@ljchang
Copy link
Member

ljchang commented May 4, 2020

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
Copy link
Collaborator

ejolly commented May 4, 2020

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.

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

3 participants