-
Notifications
You must be signed in to change notification settings - Fork 44
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
Char incompatibility size #107
Comments
You need to use |
Ah, I see that |
@Barisevrenugur I see that the line in question is from your orientation check code: https://github.com/hMRI-group/hMRI-toolbox/blob/a03439b42b3a639f50fdea8283e2860e6b91c43e/hmri_create_MTProt.m#L1185C8-L1185C26 I think it does have a bug if the MTw, PDw, and/or T1w files have different file name lengths. Could you test it (e.g. with a PDw file named |
The exact error I get is: |
How are you calling |
Dear @franciscofritz, this line of code that i believe i implemented during hackathon was already tested but possibly with file names with the same length (but then as far as i checked now, all sets of test data that I posses do have the same file name length!). With @lukeje , we made a quick test with files having different name lengths but the toolbox executed without any problem, we did not reproduce the problem you reported above. As we discussed, this happens (file names with differing length) particularly with the BIDS format of the MT file names which you also have above as example: the files otherwise having names with the same length end up having names of different lengths due to addition of |
That's very interesting; it seems like the rfsens code converts to @Barisevrenugur Could you confirm whether your testing includes per-contrast rfsens correction, and if not, see whether you can reproduce Frank's problem? |
Ah, the confounding factor is that all our filenames in the testing are the same length. Therefore the stacking of |
Yeah, for now I did a quick hack in hmri_create_MTprot for now. |
the line you reported at the beginning:
works without any problem with same-length and differing-length filenames, as tested manually by me and also by me/Luke as I wrote above. in the below output (manual test): the beginning is the content of raw in the succesfull testing with different-length file names and below line shows that raw is a 8x1 cell array-as intended, not a char array):
the toolbox is tested both with per contrast and also US. So, we do test it with per contrast RFsens correction as well, however our tests were succesfull. I did not check it specifically for RFsens maps but yes (as I clarified above) all sets of test data we possess have same length names: this is a standard, there is nothing wrong with it. The line of code (the MTprot line above works fine with different length file names- as tested several times) that would lead to the above problem with differing file names is this one in RF sensitivity correction (as you also quoted above): hMRI-toolbox/hmri_create_RFsens.m Line 98 in ffa8444
This I have not directly/manually tested yet but can say rightaway that it would cause the above quoted problem. The simple solution would be to change the char array on this line with a cell array which is congruent with the MTprot code. It is OK to change this because |
@Barisevrenugur Please try and replicate the issue before changing the code; if you can't replicate it, then you can't test whether any fix works. I would suggest running the toolbox with the BIDS-ified version of the test dataset ( @franciscofritz Could you make a pull request with your hack so we can test it? |
I see another reasonable change; we could change these lines to be raw = char(char(jobsubj.raw_mpm.MT), char(jobsubj.raw_mpm.PD), char(jobsubj.raw_mpm.T1));
[orientationsMatch, orientationsWarning] = hmri_check_nifti_orientations(spm_vol(raw)); This way of doing things makes clear that we want |
Dear @franciscofritz , please give a quick feedback as to whether the linked (tested) PR runs your data without a problem as well. |
Hi @Barisevrenugur - I am currently in mental sick leave and also I don't have access to a working Matlab environment right now to give it a test. Is it possible to do when I am back? (probably end of the month). |
Hi @franciscofritz , of course! yes, please take your time to get well and let us know when you have chance to look into it. I hope you get well soon and wish you a speedy recovery. |
I cannot calculate the MPM parameters because it is a problem with the vertical concatenation of the my MPM contrast files.
Specifically, this is my input of my MPM contrast files:
And the error appears in line 1185 of the code "hmri_create_MTProt (internal function: get_mpm_params)":
raw = [jobsubj.raw_mpm.MT; jobsubj.raw_mpm.PD; jobsubj.raw_mpm.T1];
Where now the jobsub.raw_mpm structures are char structure are 6x162, 6x163 and 6x163; I can see the problem. What I don't understand is why it is forced that the inputs must be the same character size rather than just strings. Any suggestion (that is not changing the names) would be appreciated.
The text was updated successfully, but these errors were encountered: