-
Notifications
You must be signed in to change notification settings - Fork 272
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
add outlier functions to bypass matlab's isoutlier; adapt to r2017b #263
Conversation
To be clear, the "inconsistency" is just that the |
Fair enough. |
Should we consider updating the MCR version that the compiled matlab uses? Or do we not want to go there...? |
I think the reason why we keep using the r2017b is because the non-python version of FIX is no longer being updated and it is using r2017b runtime. Perhaps this raises another question if we should switch to the pyFIX at some point in the future... |
Yes, the container and related environment variables would need to support two different MCRs. But that was in fact the situation at one point previously, so its doable. |
At some point, given the amount of Matlab we have other than FIX, we are going to want to update the Matlab MCR version, although maybe it does make sense to wait to do that (if we can wait) until we switch over to using pyFIX. |
This looks reasonable to me. Any other opinions? |
I am okay with committing if this is tested and Tim approves it. |
I have tested it and the new feature csv is generated here without error: /media/myelin/brainmappers/Connectome_Project/YA_HCP_Final/S1200_MSMAll3T535T/MNINonLinear/Results/rfMRI_REST/tICA_d84/features.csv |
I'll need to recompile it against r2017b, preferably before merging. |
If you want me to test the compiled version, please let me know when it's ready. |
I have pushed a commit with that function recompiled, try that in compiled mode. |
I guess I just need to |
Yes. |
Hi Tim, I get an error
The error log is saved here |
I would guess you have the MCR set incorrectly in your SetUp... script, /export isn't where we put stuff on our lab machines. I don't know if we have MCR v93 installed on them. |
Oh, I see. There's nothing under this path |
Try |
Here's another error.
|
Hi Tim, I made a minor change to the |
I haven't seen that "can't reload" error before. I could install the MCR locally instead of on nfs, and see if that helps. |
Try /home/brainmappers/MCR_install/v93 on desktop4. I am recompiling now. |
I will try to run after I get the updated compiled script, given that the old version takes about 1hr to run to where the error occurs. |
I tested the rewritten version of aryule using the following script,
Here're the results,
So, I think this rewritten |
Have you tested tICA in compiled mode with it? |
Unfortunately, it seems that it have solved the aryule issue but there's another issue regarding the function
|
This is weird. I wrote this part of code based from https://github.com/Washington-University/HCPpipelines/blob/master/global/matlab/nets_spectra/nets_spectra.m#L37 and now it gives me an error. |
I compiled your aryule test and ran it on my linux machine, and it worked. I then tried it on desktop5, which failed with the reload error. So, it seems there is something about the OS version or packages that is causing this reload error. |
What a relief! (especially after I found pwelch is hard to rewrite...) Is it the raw aryule or the myfunc_aryule that you have tested on your machine? |
Matlab's original aryule. |
Qunex is currently using CentOS 7, I believe, which is older that what we have on desktop5, so it is possible compiled things will work in it without even replacing aryule. However, this implies that tICA in compiled mode will stay broken for modern linux until we use something newer than MCR v9.3 (and any other matlab in the pipelines that uses something more complicated than pinv may also already be broken for modern linux). |
Since the user has tested it without error, I would suggest we can merge this PR now. |
I am okay with that if Tim approves. |
Background
Matlab's
isoutlier
function has new optionpercentile
that wasn't introduced in r2017b but in the latest versions. Because of this, the compiled version of Matlab always throws errors for scripts written in r2022 which use commands such asisoutlier(a, 'percentile', [5,95])
.Main changes
isoutlier
andfilloutlier
are removed from the tICA feature generation stepfindFivePercentageOutliers
to find outliers outside 5% to 95% rangefindScaledMADOutliers
to find outliers three scaled MAD away from the medianEvaluation
In r2022b (raw version),
in r2022b & r2017b (current commit),