Incorporating json into bids.File (issues #596 #371)

[nbeliy]

First draft to incorporating managing sidecar into bids.File class.

@Remi-Gau , Is it better to use the bids.File class directly or use a daughter class extending functionality?

Todo:

• lint
• tests
• docs
• Decide if create new class bids.JSONFileor use the bids.File
• Add path management to Sources and Raw. BIDS requires path starting from root of dataset

---

• closes [ENH]: Incorporate json manipulation into bids.File class #596
• Closes Add a method to bids.File for changing file metadata #371

[Remi-Gau]

Is it better to use the bids.File class directly or use a daughter class extending functionality?

my hunch would be to first keep things in the same class

[nbeliy]

Ok, will merge into main class.

[nbeliy]

Few changes:

• Removed JSONFile subclass, and moved functions to File class
• Created new util.update_struct function that manages the updating fields in json structure (with tests)
• Added documentation

@Remi-Gau

• util.update_struct duplicates the internal function from bids.Description, should we replace it?
• In current bids.File implementation, the metadata is loaded when instance is created, which may cause slowdowns (reading file on disk and searching for inheritence). Will it be better to make metadata load on request only?
• I've added the doc for both util.update_struct and File, can you check if it's extensive and comprehensive?

[Remi-Gau]

Suggested change

	% 'b-add', 'val_b2')
	% 'b-add', 'val_b2')

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" ?

[Remi-Gau]

maybe it would be better to have simpler function:

• one for adding
• one for removing
• one for appending an element to a an array or a cell
• one for popping an element from a array or a cell

or have the first argument specify what "action" to perform.

[nbeliy]

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:

 [file, val] = file.metadata_pop('val_a');

What is your take on not forcing to read metadata when creating File instance, and making it on request only?

[Remi-Gau]

the fieldnames like b-add will be forbidden

Oh dear... Matlab 101. Silly me for forgetting this... I guess that's what happens when you start switching back and forth between languages

[Remi-Gau]

What is your take on not forcing to read metadata when creating File instance, and making it on request only?

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 of tmp/WIP/sub-01_task-foo_bold.json.

temp
├── task-foo_bold.json
└── WIP
    ├── sub-01_task-foo_bold.json
    └── sub-01_task-foo_bold.nii

I think this was the kind of concern I had. But improving the handling of the inheritance principle could help with things like this.

[nbeliy]

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.

[Remi-Gau]

Just realized that set_metadata relies on bids.internal.get_meta_list that tries to implement the inheritance principle (same one use for bids.layout).

    function obj = set_metadata(obj)
      if isempty(obj.metadata_files)
        pattern = '^.*%s\\.json$';
        obj.metadata_files = bids.internal.get_meta_list(obj.path, pattern);
      end
      obj.metadata =  bids.internal.get_metadata(obj.metadata_files);
    end

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.

[Remi-Gau]

For poping (remove and return) is more complicated, as matlab do not modify current object, and multiple returns looks ugly:

 [file, val] = file.metadata_pop('val_a');

I think I would just return the file object.

file = file.metadata_pop('FieldToPopFrom', 'value_to_pop');

[Remi-Gau]

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.

[nbeliy]

Added tests for File and set up implicit metadada reading.

For me, the PR can be removed from draft.

[Remi-Gau]

Tacking #369 feels a bit too much in this PR, no?
maybe better in a separate one, no?

[nbeliy]

Tacking #369 feels a bit too much in this PR, no?
maybe better in a separate one, no?

Technically yes, but it can be integrated in the current PR. But I'm not sure what to do with inheritance principle -- renaming can easily brake it.

[Remi-Gau]

You are right. One headache at time. Let's leave it out of the scope of this one for now.

[nbeliy]

1. Added functions that acts on single field:

• metadata_add(obj, field, value) to add/overwrite -- pointless, as same effect can be achieved with f.metadata.Filed = value
• metadata_append(obj, field, value) -- transforms value to cellarray if needed, and appends new value
• metadata_remove(obj, field) -- removes field from metadata, don't want to call it pop, cause in python remove removes element, and pop removes and returns element

The metada_update is kept, as a way to update several fields at same time without copying same variable 25 times (why matlab don't have pointers or references????)

Updated doc and tests (couldn't run coverage).

At this point the PR do what is supposed to do -- ready for final discussion/review?

I'll open a new tangent issue for automatic loading of metadata, just to keep things organized.

[Remi-Gau]

probably better to have smaller independent tests (into separate functions)

not a blocker: I can take care of it in a separate PR

[nbeliy]

Can do -- I put there to not reread metadata again.

[Remi-Gau]

yeah that's always the trade off, but unless set up is super heavy then I tend to prefer smaller independent tests.

[nbeliy]

Just to be clear, do you want a new function inside tests_bids_file.m or completely separate test suite? I guess it's former, but want to be sure.

[Remi-Gau]

a new function inside tests_bids_file.m

just new function

matlab is annoying enough to force you to have one "main" function per file, let's not feed the monster. 😉

[Remi-Gau]

2 minor points regarding having smaller tests and where the doc for each method should go.

If you don't have the time I can take care of it.

just let me know.

thanks

[nbeliy]

Spitted the tests.

Just to be completely transparent and to be sure that docs are clear:
metadata_update: several keys can be passed, or one structure
metadata_add, metadata_append, metadata_remove: only one key can be passed; do not catches errors
metadata_write: passed keys modifies only written metadata, not the original object

[Remi-Gau]

All good.
thanks for this @nbeliy
looking forward to start using this actually!!

[Remi-Gau]

I may add another option later in a separate PR to add the possibility to use the inheritance principle or not when loading the metadata

will ask for a review if you have time