move detpar to detector arrays #784
Conversation
| expinf2 = sqw_copy.experiment_info; | ||
|
|
||
| % all instruments changed with new name to make expinf different | ||
| for i=1:numel(expinf1.instruments), expinf1.instruments{i}.name = 'copy'; end |
There was a problem hiding this comment.
Personally, I really dislike inline dos/ifs like this. Definitely a personal matter.
There was a problem hiding this comment.
expinf1.instruments = cellfun(@(x) setfield(x, 'name', 'copy'), expinf1.instruments, 'UniformOutput', false)might be cleaner? Though admittedly MATLAB's verbosity in making the output a cell-array this sort of defeats that.
There was a problem hiding this comment.
Happy to change the formatting. Arguable if the cellfun version is cleaner - it is very opaque and in the interests of making clear what a test does if there is the problem here I would retain the for loop.
| @@ -295,6 +300,15 @@ | |||
| head = [head{:}]; | |||
| head = rmfield(head,{'instrument','sample'}); | |||
| end | |||
|
|
|||
| function dp = get.detpar(obj) | |||
| if numel(obj.detector_arrays)>0 | |||
There was a problem hiding this comment.
Is this not equivalent to ~isempty which denotes the meaning more clearly
| @@ -134,7 +134,8 @@ | |||
| % ---------------------------------------------------------------------- | |||
| d.main_header=main_header; | |||
| d.experiment_info=header; | |||
| d.detpar=det0; | |||
| d.experiment_info.detector_arrays(end+1) = IX_detector_array(det0); | |||
| d.detpar=struct([]); | |||
There was a problem hiding this comment.
In keeping with other changes would struct.empty be better?
| @@ -88,7 +88,11 @@ | |||
| wout = noisify(w,varargin); | |||
|
|
|||
| function dtp = my_detpar(obj) | |||
| dtp = obj.detpar_x; | |||
| if ~isempty(obj.detpar_x) | |||
There was a problem hiding this comment.
If both are empty this will error. Expected?
There was a problem hiding this comment.
obj in this case is a sqw (or array thereof). Guess it is conceivable that obj could be an empty array with the right type. If it errors maybe we rewrite in python?
| dd = IX_detector_array(S.detpar); | ||
| S.experiment_info.detector_arrays = IX_detector_array.empty; | ||
| S.experiment_info.detector_arrays(end+1) = dd; | ||
| S.detpar = struct([]); |
| % the generic code is preceded by the conversion of old-style | ||
| % detpar info into the new detector arrays. This allows loading | ||
| % of .mat files without converting them on disk.. | ||
| if isfield(S,'experiment_info') && isfield(S,'detpar') |
There was a problem hiding this comment.
Just tested it and you can do
if isfield(S, {'experimental_info', 'detpar'})| @@ -253,7 +253,8 @@ function check_obj_initated_properly(obj) | |||
| function check_error_report_fail_(obj,pos_mess) | |||
| % check if error occured during io operation and throw if it does happened | |||
| [mess,res] = ferror(obj.file_id_); | |||
| if res ~= 0; error('SQW_FILE_IO:io_error',... | |||
| if res ~= 0 | |||
| error('SQW_FILE_IO:io_error',... | |||
There was a problem hiding this comment.
Horace:SQW_FILE_IO:io_error
| if isa(headers, 'Experiment') | ||
| dp = obj.get_detpar(); | ||
| headers.detector_arrays(end+1) = IX_detector_array(dp); | ||
| sqw_struc.detpar = struct([]); |
There was a problem hiding this comment.
essential remark is about loadobj.
Everything else looks like work in progress.
Think about exposing important properties through public interface. All detector properties (id, x2 psi, phi, ... etc should be top level properties, returning appropriate arrays as the function of a run number. I think, it will greatly simplify the usage of a compressed arrays of instruments in a future
Also do not see changes in unit tests. Its unlikely changes in property from [] to IX_detector_array.empty would not affect a unit test
| assertEqual(numel(sqw_obj.experiment_info.detector_arrays.id), 36864); | ||
| assertEqual(numel(sqw_obj.data.pax), 1); |
There was a problem hiding this comment.
Why to change interface in such convoluted way?
why not to do
sqw_obj.experiment_info.id and wire up this property to the array internally
| obj = Experiment(IX_detector_array.empty, instruments, samples); | ||
| obj.expdata = expdata; |
There was a problem hiding this comment.
I would allow old interface (user would not be so keen to learn about IX_detector_array.empty as [] looks much easier ) and make the internal assignment to IX_detector_array.empty as this happen
| d.experiment_info.detector_arrays(end+1) = IX_detector_array(det0); | ||
| d.detpar=struct([]); |
There was a problem hiding this comment.
would not work without array compression and will be very inefficient, as det0 and IX_detector_array are large objects
| % of .mat files without converting them on disk.. | ||
| if isfield(S,'experiment_info') && isfield(S,'detpar') |
There was a problem hiding this comment.
should all this be moved to from_old_struct and loadobj left untouched? It looks like you are verifying old structure processing here, and this should not happen in normal circumstances. New structure will eventually be prevailing, leaving old data input an exception.
It is planned to move the detector info from the separate detpar field in sqw with an array of IX_detector_arrays in the experiment_info field. This PR does this for Horace, initially with one IX_detector_array only. The parallel Herbert PR completes the process.
SL7 branch build #43 combines this with the Horace PR succesfully