Add BIDSJSONFile #444

tyarkoni · 2019-05-14T17:29:58Z

Adds a BIDSJSONFile to the BIDSFile hierarchy. This introduces .get_dict() and .get_json() methods that return the contents of a .json file. See #442 for discussion.

yarikoptic

Just added a few suggestions.

So this is only a preamble to get #442 "addressed"?

(git-annex)hopa:~/datalad/openfmri/ds000001[master]git
$> python -c "from pprint import pprint; import bids; b = bids.BIDSLayout('.'); print('Version: ', bids.__version__); pprint(b.get_metadata('dataset_description.json'))"
('Version: ', '0.8.0+208.g5860e4f')
{}
$> python -c "from pprint import pprint; import bids; b = bids.BIDSLayout('.'); print('Version: ', bids.__version__); pprint(b.get_metadata('$PWD/dataset_description.json'))"
('Version: ', '0.8.0+208.g5860e4f')                                                                                                          
{}

bids/layout/models.py

yarikoptic · 2019-05-14T18:13:45Z

bids/layout/tests/test_models.py

+    j = jf.get_json()
+    assert isinstance(j, six.string_types)
+    assert 'RepetitionTime' in j
+    assert json.loads(j) == d


Adding a test specifically on dataset_description.json would be great

codecov · 2019-05-14T18:26:35Z

Codecov Report

Merging #444 into master will increase coverage by 0.89%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #444      +/-   ##
==========================================
+ Coverage   82.61%   83.51%   +0.89%     
==========================================
  Files          23       23              
  Lines        2848     3039     +191     
  Branches      718      829     +111     
==========================================
+ Hits         2353     2538     +185     
- Misses        314      321       +7     
+ Partials      181      180       -1

Flag	Coverage Δ
#unittests	`83.51% <91.66%> (+0.89%)`	⬆️

Impacted Files	Coverage Δ
bids/layout/__init__.py	`100% <100%> (ø)`	⬆️
bids/layout/layout.py	`87.81% <100%> (+3.66%)`	⬆️
bids/layout/models.py	`89.55% <80%> (-0.4%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a833393...2ee727a. Read the comment docs.

Co-Authored-By: Yaroslav Halchenko <[email protected]>

tyarkoni · 2019-05-14T18:34:36Z

Re: getting #442 addressed, I'm not sure I follow... as I said in the other thread, I see the fact that get_metadata() returns nothing now as expected behavior. The metadata contained inside JSON sidecars describes other files, not the JSON file itself. So to my mind it makes sense that calling get_metadata() on a JSON file should return nothing. The .get_dict and get_json methods are intended as substitutes for get_metadata in this particular use case, not as an implementation layer.

yarikoptic · 2019-05-14T19:28:35Z

ah, ok, gotcha.

The metadata contained inside JSON sidecars describes other files, not the JSON file itself.

although generally the case, with dataset_description.json I could argue it is different since it describes the entire dataset, so this metadata is relevant to all files, including meta- data in the dataset, and thus itself as well.

So now I would need to use explicitly BIDSJSONFile('./dataset_description.json').get_dict() to get metadata pertinent to entire dataset or there is another shorter/easier way?
Sorry for the RTFM level questions ;)

tyarkoni · 2019-05-14T19:34:43Z

For the special case of dataset_description.json, I think it's reasonable to have a BIDSLayout-level method, as that's a special file in the spec itself. What do you think it should be called? get_dataset_description()?

Edit: or maybe just .describe()?

yarikoptic · 2019-05-14T20:21:07Z

I kinda liked get_dataset_description since it is matching what it would do ;)

But I also wondered, that get_metadata without path being provided (change to have path=None in get_metadata signature) could serve the same purpose and make sense. So without specific path - get dataset level metadata. Question though -- would it stay consistent with how information is queried/presented from derivatives/, i.e. e.g. how would then I get such description about specific derivative? may be then if path points to a top directory of the dataset?

tyarkoni · 2019-05-14T21:31:13Z

I'm not crazy about the idea of making get_metadata() with path=None return layout-level metadata. Especially because you're right that it would make getting derivatives descriptions complicated. If we add a get_dataset_description() method, we can then just add a scope argument that wraps the _get_layouts_in_scope helper, which is the pattern I use everywhere else. I think this would be both more consistent with the existing API and cleaner implementationally.

into bids-json-file

tyarkoni · 2019-05-15T13:45:01Z

@yarikoptic please check to make sure this implementation of get_dataset_description() works for you.

tyarkoni · 2019-07-08T15:55:32Z

@yarikoptic just pinging you about this... totally fine if you don't have time to look it over. I'm happy with it and can merge as-is.

effigies · 2019-07-10T16:23:14Z

@yarikoptic Care to have another review?

yarikoptic · 2019-07-10T16:36:23Z

Oh, sorry for missing the pings! I will check how things work out for datalad-neuroimaging later today - away from the laptop ATM

effigies · 2019-07-11T19:01:02Z

Hi @yarikoptic, did you get a chance to test this out?

yarikoptic · 2019-07-11T21:01:06Z

doing it now, if you don't hear from me within an hour -- forget I exist and proceed forward and I will show my "pretty" face later on ;-)

yarikoptic · 2019-07-11T21:29:27Z

ok, initial wave -- made datalad-neuroimaging compatible with current branch by using get_dataset_description and tuning up the tests. PR: datalad/datalad-neuroimaging#70

will try now on some openneuro dataset to see what potentially has changed in the output

yarikoptic · 2019-07-11T21:53:23Z

ran into some other gotchas, will report on that separately -- don't wait! carry on (feel welcomed to merge, sorry for the delays!)

effigies · 2019-07-11T22:33:08Z

Sounds good. Thanks for the review.

yarikoptic · 2019-07-11T22:35:04Z

just for my own sake -- tested on ds00258. Differences from using 0.8 just added extension and absent probably bogus super precise float

@@ -4456,10 +4458,11 @@
       2.38766666667,
       1.15266666667
     ],
-    "SpacingBetweenSlices": 4.840000000000001,
+    "SpacingBetweenSlices": 4.84,
     "TaskName": "rest",
     "datatype": "func",
     "echo": "2",
+    "extension": "nii.gz",
     "subject": "04570",
     "suffix": "bold",
     "task": "rest"

differences from some prior (0.5...?) version (to 0.8 and this) include loss of metadata from derivatives folders... I guess we needed to adjust more ;-)

@effigies

Version 0.9.2 (July 12, 2019) This version includes a number of minor fixes and improvements. EEG files are better handled, and `BIDSLayout` and `BIDSFile` play more nicely with `Path`-like objects. With thanks to new contributor Cecile Madjar. * FIX: Instantiate `ignore`/`force_index` after root validation (bids-standard#457) * FIX: Restore `<entity>=None` query returning files lacking the entity (bids-standard#458) * ENH: Add `BIDSJSONFile` (bids-standard#444) * ENH: Add `BIDSFile.__fspath__` to work with pathlib (bids-standard#449) * ENH: Add `eeg` datatype to layout config (bids-standard#455) * RF: Remove unused kwargs to BIDSFile (bids-standard#443) * DOC: Improve docstring consistency, style (bids-standard#443) * DOC: Address final JOSS review (bids-standard#453) * STY: PEP8 Fixes (bids-standard#456) * MAINT: Set name explicitly in setup.py (bids-standard#450) * tag '0.9.2': (318 commits) MAINT: Update changelog, Zenodo ordering fix(BIDSLayout): more readable query building tst(BIDSLayout): add two more tests to the entity=None case FIX: Allow None to select for the absence of a tag TEST: Verify entity=None fails to find files lacking that entity FIX: Instantiate ignore/force_index after root validation fix test revert turning '==' into 'is' pep8 fixes fix typo in docstring fix typo and added eeg to the config bids.json file TEST: Expect failure on Python < 3.6 TEST: Verify Path can accept a BIDSFile ENH: Add BIDSFile.__fspath__ to work with pathlib DOC: Fix BIDS citation DOC: Spaces around emdashes, italicize e.g., i.e. DOC: Drop Zenodo reference DOC: Update bibliography file with eye to formatting fix name in setup.py so used-by count works; h/t @effigies DOC: Add PyPI badge for latest version available ...

Now that bids-standard/pybids#444 was merged/released

tyarkoni added 2 commits May 14, 2019 12:28

adds BIDSJSONFile; closes #442

7cc2929

add BIDSFile subclasses to namespace

5860e4f

tyarkoni requested a review from yarikoptic May 14, 2019 17:30

yarikoptic reviewed May 14, 2019

View reviewed changes

ensure JSON contains a top-level dict, not list

24f6e1c

Co-Authored-By: Yaroslav Halchenko <[email protected]>

tyarkoni added 3 commits May 15, 2019 08:24

add 'self' option to _get_layouts_in_scope

f73379d

add get_dataset_description method

53fb7ba

Merge branch 'bids-json-file' of https://github.com/bids-standard/pybids

a5b8f3b

into bids-json-file

tyarkoni and others added 2 commits July 8, 2019 17:58

Merge branch 'master' into bids-json-file

c8a4ca0

Merge branch 'master' into bids-json-file

2ee727a

effigies added this to the 0.9.2 milestone Jul 10, 2019

yarikoptic mentioned this pull request Jul 11, 2019

BF: be compatible with upcoming pybids > 0.9.1 datalad/datalad-neuroimaging#70

Merged

effigies merged commit 7f7e039 into master Jul 11, 2019

effigies deleted the bids-json-file branch July 11, 2019 22:33

yarikoptic added a commit to yarikoptic/datalad-neuroimaging that referenced this pull request Aug 12, 2019

RF: demand pybids >=0.9.2.

92571e2

Now that bids-standard/pybids#444 was merged/released

yarikoptic mentioned this pull request Aug 12, 2019

RF: demand pybids >=0.9.2. datalad/datalad-neuroimaging#72

Merged

yarikoptic added a commit to yarikoptic/datalad-neuroimaging that referenced this pull request Aug 13, 2019

RF: demand pybids >=0.9.2.

d54ec42

Now that bids-standard/pybids#444 was merged/released

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BIDSJSONFile #444

Add BIDSJSONFile #444

tyarkoni commented May 14, 2019

yarikoptic left a comment

yarikoptic May 14, 2019

codecov bot commented May 14, 2019 •

edited

Loading

tyarkoni commented May 14, 2019

yarikoptic commented May 14, 2019

tyarkoni commented May 14, 2019 •

edited

Loading

yarikoptic commented May 14, 2019

tyarkoni commented May 14, 2019 •

edited

Loading

tyarkoni commented May 15, 2019

tyarkoni commented Jul 8, 2019

effigies commented Jul 10, 2019

yarikoptic commented Jul 10, 2019

effigies commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

effigies commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

Add BIDSJSONFile #444

Add BIDSJSONFile #444

Conversation

tyarkoni commented May 14, 2019

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic May 14, 2019

Choose a reason for hiding this comment

codecov bot commented May 14, 2019 • edited Loading

Codecov Report

tyarkoni commented May 14, 2019

yarikoptic commented May 14, 2019

tyarkoni commented May 14, 2019 • edited Loading

yarikoptic commented May 14, 2019

tyarkoni commented May 14, 2019 • edited Loading

tyarkoni commented May 15, 2019

tyarkoni commented Jul 8, 2019

effigies commented Jul 10, 2019

yarikoptic commented Jul 10, 2019

effigies commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

effigies commented Jul 11, 2019

yarikoptic commented Jul 11, 2019

codecov bot commented May 14, 2019 •

edited

Loading

tyarkoni commented May 14, 2019 •

edited

Loading

tyarkoni commented May 14, 2019 •

edited

Loading