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.

Sign up for GitHub

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

Jump to bottom

MATLAB Improvements #219

Merged
merged 5 commits into from
Jun 5, 2020
Merged

MATLAB Improvements #219

merged 5 commits into from
Jun 5, 2020

Conversation

MartinK84
Copy link
Contributor

I have given the MATLAB wrappers some love and implemented the following improvements:

Major Changes

  • The workaround of clearing LD_LIBRARY_PATH (Linux) and DYLD_LIBRARY_PATH (Mac) is now only applied temporarily and the variables are restored to their original value at the end of the bart function. This is important as the clearing of those variables could influence or break other MATLAB functions.
  • The varargin input variables are now split into character and non-character variables. The character variables are appended to the given bart command and the non-character variables are written as files and used as input. This change enables usage of commands such as "bart" or "bart fft -h" directly from within MATLAB, e.g.
>> bart fft -h
Usage: fft [-u] [-i] [-n] bitmask <input> <output>

Performs a fast Fourier transform (FFT) along selected dimensions.

-u		unitary
-i		inverse
-n		un-centered
-h		help

Minor Changes

  • added sectioning comments to better structure bart.m
  • cleanup of formatting warnings
  • better documentation for bart.m, writecfl.m and readcfl.m when using the MATLAB help command
  • renamed WSLPathCorrection to lower case wslPathCorrection for file naming consistency

@MartinK84
Renamed WSLPathCorrection to lower case for consistency
9755103
@MartinK84
Better documentation when using the help command
1d76a78
@MartinK84
Changed LD_LIBRARY_PATH (linux) and DYLD_LIBRARY_PATH (mac) to be cle…
64502ea 
…ared only temporarily and to be restored at the end of the bart call

Added separation of varargin inputs into character inputs and data inputs. Character inputs are then appended to the given bart command. This enables calling of only "bart" or "bart traj -h" commands from within matlab, e.g. when no data input is given
Renamed loop variables ("i" should not be used as it represents the complex number)
Better function description when using the help command
Added section comments and cleaned up formatting warnings
johannestoger
johannestoger reviewed Jun 2, 2020
View reviewed changes
matlab/bart.m Outdated Show resolved Hide resolved
johannestoger
johannestoger reviewed Jun 2, 2020
View reviewed changes
matlab/bart.m Outdated

% Check input variables
if nargin==0 || all(cmd==0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should all(cmd==0) actually be isempty(cmd)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all(cmd==0) does seem to work, but is not as elegant, true. I have changed it to isempty(cmd)

johannestoger
johannestoger reviewed Jun 2, 2020
View reviewed changes
matlab/bart.m
if isempty(bart_path)
if exist('/usr/local/bin/bart', 'file')
bart_path = '/usr/local/bin';
elseif exist('/usr/bin/bart', 'file')
bart_path = '/usr/bin';
else
% Try to execute bart inside wsl, if it works, then it returns
% status 0
% Try to execute bart inside wsl, if it works, then it returns status 0
[bartstatus, ~] = system('wsl bart version -V');
if bartstatus==0
bart_path = '/usr/bin';
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see, the variable bart_path is actually not used for the WSL code branch below.

Copy link
Contributor Author

@MartinK84 MartinK84 Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That is correct, I'm running bart on a native ubuntu, not using wsl - so I did not bother checking. Nevertheless, I changed the code to use bart_path as well in the wsl part, see 69e988d

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but how do we know that bart is in /usr/bin ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't, as far as I understand, this probably very ancient piece of code, it just assumes this as default location in case the TOOLBOX_PATH is not set. Because WSL 2 is supposed to be released soon (https://docs.microsoft.com/en-us/windows/wsl/wsl2-index this will probably require an overhaul with WSL 2 anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it. I might play with it once I upgrade to WSL 2.

@uecker
Copy link
Member

uecker commented Jun 3, 2020

Can someone check with octave?

@hcmh
Copy link
Member

hcmh commented Jun 4, 2020

Can someone check with octave?

Is there something specific you want checked? The interface still works with octave on Linux.

@MartinK84
Copy link
Contributor Author

I have also just tried it on a fresh install of octave 5.2.0 (never even used octave before) and a quick test appeared to have worked out of the box:

>> addpath(fullfile(pwd, 'matlab'));
>> setenv('TOOLBOX_PATH', pwd);
>> bart
Usage: bart <command> <arguments...>

BART. Available commands are:
avg         bench       bin         bitmask     cabs        caldir
calmat      carg        casorati    cc          ccapply     cdf97
circshift   conj        conv        copy        cpyphs      creal
crop        delta       ecalib      ecaltwo     estdelay    estdims
estshift    estvar      extract     fakeksp     fft         fftmod
fftrot      fftshift    filter      flatten     flip        fmac
homodyne    index       invert      itsense     join        looklocker
lrmatrix    mandelbrot  mip         moba        nlinv       noise
normalize   nrmse       nufft       ones        pattern     phantom
pics        pocsense    poisson     poly        repmat      reshape
resize      rmfreq      rof         rss         rtnlinv     sake
saxpy       scale       sdot        show        slice       spow
sqpics      squeeze     ssa         std         svd         tgv
threshold   toimg       traj        transpose   twixread    upat
var         vec         version     walsh       wave        wavelet
wavepsf     whiten      window      wshfl       zeros       zexp
>> bart phantom -h
Usage: phantom [-s d] [-S d] [-k] [-t <string>] [-G ...] [-T ...] [-B ...] [-x d] [-g d] [-3] [-b] <output>

Image and k-space domain phantoms.

-s nc           nc sensitivities
-S nc           Output nc sensitivities
-k              k-space
-t file         trajectory
-G              geometric object phantom
-T              tubes phantom
-B              BART logo
-x n            dimensions in y and z
-g n=1,2        select geometry for object phantom
-3              3D
-b              create basis for geometry
-h              help
>> data = bart('phantom');
>> size(data)
ans =

   128   128

@MartinK84 MartinK84 mentioned this pull request Jun 4, 2020
Closed
@uecker uecker merged commit a82d5b7 into mrirecon:master Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johannestoger johannestoger johannestoger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MartinK84 @uecker @hcmh @johannestoger