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.

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

MATLAB Wrapper and tools that output the result with bart_printf #228

Open
fyrdahl opened this issue Jul 8, 2020 · 8 comments
Open

MATLAB Wrapper and tools that output the result with bart_printf #228

fyrdahl opened this issue Jul 8, 2020 · 8 comments

Comments

@fyrdahl
Copy link
Contributor

fyrdahl commented Jul 8, 2020

I ran into an issue with the MATLAB wrapper when using tools that output the result with bart_printf, in particular estdelay. Ideally, I would like to get the output as a variable in MATLAB. Using out = bart('estdelay',traj,data) results in an error because there is no output from estdelay. I'm aware that I could capture the command window output by doing something like out = evalc("bart('estdelay',traj,data)"); or but it's not very tidy.

I made a very hacky attempt to have bart.m attempt to drop theout_str and try again if it detects a specific error message. Whereas it doesn't seem to break any code for me, it's might result in unforeseen consequences down the line. Another approach would be to have bart.m recognize these tools based on thecmd-string and handle them accordingly, i.e. calling bart without an out_str, but this doesn't seem like a very clean solution either.

@MartinK84
Copy link
Contributor

MartinK84 commented Jul 13, 2020

I can try to play around with the MATLAB wrapper and got some ideas on how to make it work. Can you post a minimal working example, best with phantom data, on how to use the estdelay command?

something like:

traj = bart(...)
data = bart(...)
out = bart('estdelay',traj,data)

Then I could take this to play around with it :-)

@fyrdahl
Copy link
Contributor Author

fyrdahl commented Jul 13, 2020

MWE as requested

traj = bart('traj -G -q1.5:1:-0.5 -y128');
data = bart('phantom -k -t',traj);
out = evalc("bart('estdelay',traj,data)"); % Works
out = bart('estdelay',traj,data); % Causes error

Ideally, I would also strip trailing line breaks from out as this would cause problems when using it.

@MartinK84
Copy link
Contributor

Looking into it I also can't see any nice solution for this, there are many limitations that really screw this up.

  1. The MATLAB wrapper has no way of knowing how many outputs each BART command expects. Just dropping the or one output in case there is an error and just blindly trying to run it again is very dangerous as it can create very unexpected and weird behavior whenever any of the BART commands fail for any reason.

  2. Having a cell array within bart.m that defines "numeric output" commands and then changes the Wrapper behavior for those might be a solution, although it increases the wrappers' complexity and the list needs to be maintained whenever new tools are added or drastically changed.

  3. Another problem I see is how bart_printf() is used by the various BART tools. There is no consistent standard of how the printed output is formatted. Some tools output the information with a prefix and some without. Furthermore, there is different separation (currently a mixture between spaces and colons is around) between the values, so even if we get the output strings into MATLAB there will be a lot of madness parsing them (that again need to be maintained).

Instead of overly complicating the MATLAB wrapper I'm just wondering if it would make sense to fix and improve the BART tools that are using bart_printf() at the moment. Would it make sense to give those tools an optional output parameter? When set the actual BART tools would just write the calculated/estimated parameters to a 1-dimensional cfl file (and even if it just contains a single value). The MATLAB wrapper would just read that file and return the values from it. And even when using BART from the command line one could use this mechanism for storing the estimated values in a file. One could alternatively allow for using just .txt files as output extension.

@sidward
Copy link
Contributor

sidward commented Jul 13, 2020

A quick note on (1): In the python wrapper, which I wrote to be very similar to the Matlab version, I had to make a similar compromise where the first argument to the "bart" function was the number of expected outputs. Maybe it is worth to bring that argument to the Matlab wrapper as well?

@MartinK84
Copy link
Contributor

A quick note on (1): In the python wrapper, which I wrote to be very similar to the Matlab version, I had to make a similar compromise where the first argument to the "bart" function was the number of expected outputs. Maybe it is worth to bring that argument to the Matlab wrapper as well?

It would, of course, be an option, but I think one would then also have to add another option to differentiate between file output parameters and variable/value output parameters like the thread started asked for (dumped to console via bart_printf()). And then things start to become more and more complicated for the user to use.

One option of course would be to have it as optional named varargin parameter like this:

out = bart('estdelay',traj,data,'DirectOutputs, 1);

but this would still not solve the problem with the non-standardized output of the functions that are using bart_printf() and the required string parsing madness to transform the grabbed stdout to a value/array. But it would at least not interfere or break with the current functionality and usage.

@uecker
Copy link
Member

uecker commented Jul 13, 2020

I do not think parsing is as bad as you think. There are only a couple of commands where it makes sense to parse the output into an array. For these, you need to remove a text prefix followed by a colon, and then either parse a series of numbers, complex numbers, or the the 3-vector %f:%f:%f

@MartinK84
Copy link
Contributor

I do not think parsing is as bad as you think. There are only a couple of commands where it makes sense to parse the output into an array. For these, you need to remove a text prefix followed by a colon, and then either parse a series of numbers, complex numbers, or the the 3-vector %f:%f:%f

Was just hoping for a more generic solution :-)
But the parsing is no problem, I can implement it - should be quite easy with MATLAB.

Then I guess the remaining question is how to implement it into the wrappers' interface, I would like to not keep the current very simple functionality. So maybe extending by an optional named varargin like out = bart('estdelay',traj,data,'DirectOutputs', 1); would work best? It would not break any functionality and the help of the function could state it as an example for the few BART commands that require it.

@uecker
Copy link
Member

uecker commented Jul 13, 2020

We were thinking about having an option to output a machine-readable specification for each command-line tool that describes input and output. This could then be used by wrappers. But I am not sure how to deal with output.

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

No branches or pull requests

4 participants