Skip to content

Update construct_stim_matrices.py#184

Open
JonathanShor wants to merge 1 commit intocvnlab:mainfrom
JonathanShor:fix_short-stim-vector-bad-indexing
Open

Update construct_stim_matrices.py#184
JonathanShor wants to merge 1 commit intocvnlab:mainfrom
JonathanShor:fix_short-stim-vector-bad-indexing

Conversation

@JonathanShor
Copy link

Guard against v[: len(v) - temp] wrapping. This could happen in the case that len(v) < prenumlag + postnumlag. Fix as mentioned in #183

Guard against v[: len(v) - temp] wrapping. This could happen in the case that len(v) < prenumlag + postnumlag.
@kendrickkay
Copy link
Member

Jonathan - thank you for taking a look at this and proposing a fix. We have a matlab-to-python port of GLMsingle that we are trying to maintain. The intention is that the Python version is an exact port of the Matlab (well, at least to identical behavior). If you are willing to clean up and/or fix the Python port of constructstimulusmatrix.m, that would be great

@kendrickkay
Copy link
Member

The matlab version is in https://github.com/cvnlab/knkutils/blob/master/mri/constructstimulusmatrices.m (it's a function declared at the bottom of the file). I will proceed under the assumption that the matlab version is correct, and so the job of the python port is to achieve identical behavior

@kendrickkay
Copy link
Member

There aren't any other ramifications of the code that we are talking about here, so if you want to clean it up, that shouldn't be a problem

@JonathanShor
Copy link
Author

Sure, happy to put some maintenance work into the file in general. My matlab is a bit rusty, but I'll take a look at the referred function and ensure they're functionally equivalent as best I can.

I'll be busy the next few days, but should have this done next week.

@kendrickkay
Copy link
Member

Hi Jonathan, I assume this fell by the way side. Did you want to do some checks? And/or do you think the merge request here is ready?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants