-
Notifications
You must be signed in to change notification settings - Fork 32
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
Incorporating json into bids.File (issues #596 #371) #597
Merged
Merged
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
3244858
First attempt to incorporate json into bids.File
6f3dded
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] faafcc4
Merge branch 'dev' into nb_jsonfile
Remi-Gau 962b9f9
lint
d409386
Merge branch 'nb_jsonfile' of github.com:nbeliy/bids-matlab into nb_j…
ea3e2ca
Added util function to update structures
1ddc2ea
Style and tests for util/update_struct.m
92b0518
Using metadata field instead of json_struct
4ecac3f
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 1c7fc84
Removed JSONFile subclass
dda7766
Merge branch 'nb_jsonfile' of github.com:nbeliy/bids-matlab into nb_j…
cbdc8d0
Added tests for metadata for File class
d9b450f
Fixed spelling in doc
0dbde71
File: Added functions that act on individual fields
67091c5
File: fixed style
d3f09c8
Splitted File.metadada tests into smaller ones
9ac9e74
Merge branch 'dev' of github.com:bids-standard/bids-matlab into nb_js…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
function js = update_struct(js, varargin) | ||
% | ||
% Updates structure with new values. | ||
% Can add new fields, replace field values, remove fields, | ||
% and append new values to a cellarray. | ||
% | ||
% Designed for manipulating json structures and will not work | ||
% on structarrays. | ||
% | ||
% USAGE:: | ||
% | ||
% js = update_struct(key1, value1, key2, value2); | ||
% js = update_struct(struct(key1, value1, ... | ||
% key2, value2)); | ||
% | ||
% Examples: | ||
% --------- | ||
% Adding and replacing existing fields: | ||
% update_struct(struct('a', 'val_a'),... | ||
% 'a', 'new_val', 'b', 'val_b') | ||
% struct with fields: | ||
% a: 'new_val' | ||
% b: 'val_b' | ||
% Removing field from structure: | ||
% update_struct(struct('a', 'val_a', 'b', 'val_b'), | ||
% 'b', []) | ||
% struct with fields: | ||
% a: 'val_a' | ||
% Appending values to existing field: | ||
% update_struct(struct('a', 'val_a', 'b', 'val_b'), | ||
% 'b-add', 'val_b2') | ||
% struct with fields: | ||
% a: 'val_a' | ||
% b: {'val_b'; 'val_b2'} | ||
% | ||
|
||
% (C) Copyright 2023 BIDS-MATLAB developers | ||
|
||
if numel(varargin) == 0 | ||
% Nothing to do | ||
return | ||
end | ||
|
||
if numel(varargin) > 1 | ||
key_list = varargin(1:2:end); | ||
val_list = varargin(2:2:end); | ||
elseif isstruct(varargin{1}) | ||
key_list = fieldnames(varargin{1}); | ||
val_list = cell(size(key_list)); | ||
for i = 1:numel(key_list) | ||
val_list{i} = varargin{1}.(key_list{i}); | ||
end | ||
else | ||
id = bids.internal.camel_case('invalidInput'); | ||
msg = 'Not list of parameters or structure'; | ||
bids.internal.error_handling(mfilename(), id, msg, false, true); | ||
end | ||
|
||
for ii = 1:numel(key_list) | ||
par_key = key_list{ii}; | ||
try | ||
par_value = val_list{ii}; | ||
|
||
% Removing field from json structure | ||
% Should use only empty double ([]) or any empth object? | ||
if isempty(par_value) && isnumeric(par_value) | ||
if isfield(js, par_key) | ||
js = rmfield(js, par_key); | ||
end | ||
continue | ||
end | ||
|
||
if bids.internal.ends_with(par_key, '-add') | ||
par_key = par_key(1:end - 4); | ||
if isfield(js, par_key) | ||
if ischar(js.(par_key)) | ||
par_value = {js.(par_key); par_value}; %#ok<AGROW> | ||
else | ||
par_value = [js.(par_key); par_value]; %#ok<AGROW> | ||
end | ||
end | ||
end | ||
js(1).(par_key) = par_value; | ||
|
||
catch ME | ||
id = bids.internal.camel_case('structError'); | ||
msg = sprintf('''%s'' (%d) -- %s', par_key, ii, ME.message); | ||
bids.internal.error_handling(mfilename(), id, msg, false, true); | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
function test_suite = test_update_struct %#ok<*STOUT> | ||
try % assignment of 'localfunctions' is necessary in Matlab >= 2016 | ||
test_functions = localfunctions(); %#ok<*NASGU> | ||
catch % no problem; early Matlab versions can use initTestSuite fine | ||
end | ||
initTestSuite; | ||
|
||
end | ||
|
||
function test_main_func() | ||
|
||
% testing with parameters | ||
js = struct([]); | ||
js = bids.util.update_struct(js, 'key_a', 'val_a', 'key_b', 'val_b'); | ||
assertTrue(isfield(js, 'key_a')); | ||
assertTrue(isfield(js, 'key_b')); | ||
assertEqual(js.key_a, 'val_a'); | ||
assertEqual(js.key_b, 'val_b'); | ||
|
||
% testing with struct | ||
test_struct.key_c = 'val_c'; | ||
|
||
js = bids.util.update_struct(js, test_struct); | ||
assertTrue(isfield(js, 'key_c')); | ||
assertEqual(js.key_c, 'val_c'); | ||
|
||
% testing update and removal of field | ||
js = bids.util.update_struct(js, 'key_c', [], 'key_a', 'val_a2'); | ||
assertFalse(isfield(js, 'key_c')); | ||
assertEqual(js.key_a, 'val_a2'); | ||
|
||
% testing concatenating as string cell | ||
js = bids.util.update_struct(js, 'key_b-add', 'val_b2'); | ||
assertEqual(js.key_b, {'val_b'; 'val_b2'}); | ||
|
||
% testing concatenating numericals | ||
js = bids.util.update_struct(js, 'key_b-add', 3); | ||
assertEqual(js.key_b, {'val_b'; 'val_b2'; 3}); | ||
end | ||
|
||
function test_exceptions() | ||
% Invalid input | ||
assertExceptionThrown(@() bids.util.update_struct(struct([]), 'key_b-add'), ... | ||
'update_struct:invalidInput'); | ||
assertExceptionThrown(@() bids.util.update_struct(struct([]), ... | ||
'key_b-add', [], ... | ||
'key_c'), ... | ||
'update_struct:structError'); | ||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random thought.
I know this is an edge case in the world of bids because we would expect the field to be camel camse but what if one of the field to update is named "b-add", do you have then do something like: "b-add-add" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better to have simpler function:
or have the first argument specify what "action" to perform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, no matter how BIDS will decide to make life needlessly complicated, but if json is transformed into matlab structure, the fieldnames like
b-add
will be forbidden.Splitting into several subcommand:
metadata_add
,metadata_append
,metadata_rm
can be implemented, and maybe is a good idea. But then it would be difficult to work with struct instead of parameters.For poping (remove and return) is more complicated, as matlab do not modify current object, and multiple returns looks ugly:
What is your take on not forcing to read metadata when creating File instance, and making it on request only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dear... Matlab 101. Silly me for forgetting this... I guess that's what happens when you start switching back and forth between languages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the metadata reading will try to implement the inheritance principle (by reading potentially matching metadatafiles in the parent directories) and I don't remember if I have put the fail safe in place to avoid "weird" behaviors.
For example it may be while doing some file wrangling that I do not want the metadata from
temp/task-foo_bold.json
to b added to those oftmp/WIP/sub-01_task-foo_bold.json
.I think this was the kind of concern I had. But improving the handling of the inheritance principle could help with things like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that inheritance should be part of bids (even if I don't like it due to all the complications). My question should the metadata be loaded implicitly or explicitly (
f.set_metadata();
).Implicit reading of metadata imposes that the logic of retrieving json files, and reading them happens every time when a new instance is created, even if it's not needed.
The way how inheritance principle is implemented is an another issue -- it requires the knowledge of the layout of dataset, which is stored in layout structure, but not in File. For me (very personal opinion), inheritance principle should be applied during creation of layout, by setting File.metadata_files, but not in File.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that
set_metadata
relies onbids.internal.get_meta_list
that tries to implement the inheritance principle (same one use for bids.layout).I would say that changing this is beyond the scope of this PR which is on updating metadata. Just thought it was good to mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would just return the file object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWI: will open a small PR to add a parameter to
bids.internal.get_meta_list
to decide to use the inheritance principle or not to list metadata files.It could then be reused here to let user decide what they want to do.