Open jcohenadad opened 4 years ago
There's several Octave compatibility issues. Can we make this issue a meta-issue to handle them all?
IMO this is a dependency for https://github.com/shimming-toolbox/shimming-toolbox/pull/67.
< matlab.mixin.SetGet
. It does two things for us:
handle
to make every object a pointer and cheap to pass aroundset(img,'Hdr',8)
, get(img,'Hdr')
so you can access fields dynamically by a string name, which I don't think we're using, and anyway I want us to move away from mutable data anyway (#71), and newer matlab has property setters which are pr
So I think we don't need to be concerned with #2, and can achieve #1 just with < handle
octave -e 'pkg install -forge dicom'
brew install gdcm
, pikaur install gdcm
(it's not in Arch but it's in the AUR), apt-get install libgdcm2.8
)pkg load dicom
at the top of every file that uses dicom ((I expect this to annoy Matlab, so it probably needs to be wrapped in an if octave ...
or users can put this in ~/.octaverc (https://stackoverflow.com/a/38002280)[ ] dicominfo(img).Manufacturer
has an extra ' '
on the end in Octave, which seems silly. Maybe the matlab version calls strip()
before returning?
strip()
we could call? We need it for [ ] Octave is stricter about putting everything that should be in its own file in its own file:
parse error near line 1 of file Ui/ShimUse.m
classdef must appear inside a file containing only a class definition
>>> classdef ShimUse < matlab.mixin.SetGet
parse_siemens_shadow
is accessing things that Octave's dicominfo doesn't provide -- and really, shouldn't be. Can we do without it?
error: structure has no member 'Private_0029_10xx_Creator'
error: called from
parse_siemens_shadow at line 29 column 12
dicominfosiemens at line 31 column 42
MaRdI at line 116 column 20
example at line 13 column 5
As annoying as MATLAB is, I think we're stuck with it for now. Once a fully working prototype exists in MATLAB then it might be worth thinking about making it compatible, or translating it to python etc.
The presumed Toolbox user is someone with access to an MRI + shim hardware; in comparison, assuming they also have access to Matlab is an incredibly small requirement.
That's a fair response! It means we can't do #67, at least not online. We can make a testing suite that runs on lab computers though.
The advantage of doing the move to Octave asap is to increase our testing coverage. Even if the project is not 100% fully-Octave compatible (but let's say, 80%), then this chunk can be included in our cloud-based testing framework.
I would be curious to know how much Octave-compatibility we can get with the current framework.
Can we make this issue a meta-issue to handle them all?
yes-- title changed
We can make a testing suite that runs on lab computers though.
we have Azure pipeline set up internally, with Matlab license, for another project. Should we go that route?
Ref: https://github.com/brendenlake/omniglot/pull/1/commits/85a191bc6983cfe486989576279317b1b085fc60
@rtopfer what do you think?