Skip to content

Conversation

@CerenB
Copy link
Collaborator

@CerenB CerenB commented Feb 10, 2021

Here it comes a hot-fix!
I added ffxDir folder in the subfolder to be called. otherwise remove-useless-betas-subfun is crashing.
Should fix the issue #293

@CerenB CerenB added the bug 🐛 Something isn't working label Feb 10, 2021
saveAndRunWorkflow(matlabbatch, 'concat_betaImg_tMaps', opt, subID);

removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps);
removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps,ffxDir);
Copy link
Collaborator

@marcobarilari marcobarilari Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps,ffxDir);
removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps, ffxDir, funcFWHM);

@marcobarilari
Copy link
Collaborator

LGTM :) thanks

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #294 (21b8f71) into dev (cbe3fee) will increase coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #294      +/-   ##
==========================================
+ Coverage   55.15%   55.20%   +0.05%     
==========================================
  Files         110      110              
  Lines        1922     1920       -2     
==========================================
  Hits         1060     1060              
+ Misses        862      860       -2     
Impacted Files Coverage Δ
src/workflows/bidsConcatBetaTmaps.m 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbe3fee...21b8f71. Read the comment docs.

@CerenB
Copy link
Collaborator Author

CerenB commented Feb 10, 2021

no no it will not work... there's another dependency, funcFWHM

@marcobarilari marcobarilari self-requested a review February 10, 2021 17:23
a bit crowded and ugly but works
end

function removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps)
function removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps, ffxDir)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps, ffxDir)
function removeBetaImgTmaps(beta_maps, t_maps, deleteIndBeta, deleteIndTmaps, ffxDir, funcFWHM)

@marcobarilari
Copy link
Collaborator

right! there you go :)

@CerenB
Copy link
Collaborator Author

CerenB commented Feb 10, 2021

uupps sorry I did it already. I will not add your changes, also I've run mh_style fix as well! thanks anyways :)

@CerenB
Copy link
Collaborator Author

CerenB commented Feb 10, 2021

Actually, the goal of subfunc is not clear to me. Do we want to delete the betas? At the current form, we do not delete all the betas because beta_maps may not give the entire beta_*.nii created. We are only deleting the betas which were used to create 4D map.
So what do we want for thus deleting to do? All the betas to be clean? If yes, why not the following:

% delete maps
  if deleteIndBeta

    % delete all individual beta maps
    fprintf('Deleting individual beta-maps ...  ');
    delete(fullfile(ffxDir, ['beta_*','.nii']))
    fprintf('Done. \n\n\n ');

  end

Is there a reason why we need to be selectively deleting some betas but others? e.g. is the user use this ffxDir for some other beta storing? if not we can delete all the betas!

not the ones only used for 4D.nii but all of it.
which does not exist in the pipeline atm.
@marcobarilari
Copy link
Collaborator

Yes, what you are saying makes sense. Though have not tested yet that part so I was not aware of that 'incomplete' behavior.

I genuinely don't know what would be the best-case scenario here.

@CerenB
Copy link
Collaborator Author

CerenB commented Feb 12, 2021

@Remi-Gau any preference? if not, I'll merge it with the deletion implemented (pls see above).

@Remi-Gau
Copy link
Contributor

I have no strong preference personally, mostly because I don't think I will ever want to delete the beta maps.
So I will let you decide what the best usecase is. :-) <---- look at me delegating the decision making process!!!! Yay!!!

@CerenB CerenB merged commit e49f33f into cpp-lln-lab:dev Feb 12, 2021
@CerenB CerenB deleted the cer-bidsConcat-hotfix branch June 8, 2021 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐛 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants