[MRG] Add tiny BIDS test dataset, fix doctests, and run it in CI #831

sappelhoff · 2021-07-06T09:21:12Z

follow up to #828
closes #829

This adds a tiny (800kb) BIDS dataset to a new tests/data directory for testing purposes.

Code for generating the data is shipped in mne_bids/tests/data/tiny_bids/code/make_tiny_bids_dataset.py, this dataset should be easily extensible in the future to add MEG, MRIs, iEEG, etc. when the need for it arises.

I make use of that dataset in the doctests, which I fix in this PR and "turn on" in CI checks.

I also use that dataset in an example, fixing #829

Adding the example dataset also showed me in its git diff that we have been writing TSV files without newline characters, violating UNIX/POSIX standards 😉 I remedied that.

Finally, this PR contains 3 "fixes" where I change json.dump to our internal _write_json, thereby simplifying some lines.

The diff is large, but most of it is the newly added data.

Merge checklist

Maintainer, please confirm the following before merging:

mne_bids/path.py

agramfort · 2021-07-06T09:32:41Z

+1 for pytest --doctest-modules

is it a lot of work?

sappelhoff · 2021-07-06T09:55:49Z

is it a lot of work?

Maybe 🤔 Let's see, I'll work on this a bit in the next days.

codecov-commenter · 2021-07-07T10:20:58Z

Codecov Report

Merging #831 (d09a1c9) into main (2d8a721) will increase coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #831      +/-   ##
==========================================
+ Coverage   94.17%   94.58%   +0.41%     
==========================================
  Files          23       23              
  Lines        3123     3123              
==========================================
+ Hits         2941     2954      +13     
+ Misses        182      169      -13

Impacted Files	Coverage Δ
mne_bids/inspect.py	`98.74% <ø> (ø)`
mne_bids/write.py	`96.13% <ø> (ø)`
mne_bids/path.py	`97.00% <100.00%> (+<0.01%)`	⬆️
mne_bids/sidecar_updates.py	`98.63% <100.00%> (-0.04%)`	⬇️
mne_bids/tsv_handler.py	`100.00% <100.00%> (ø)`
mne_bids/commands/mne_bids_report.py	`17.64% <0.00%> (+17.64%)`	⬆️
mne_bids/commands/run.py	`34.48% <0.00%> (+34.48%)`	⬆️

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 2d8a721...d09a1c9. Read the comment docs.

sappelhoff · 2021-07-07T10:22:45Z

I think the problem is that many examples rely on us having some files. So I'd have to "skip" executing most of the examples, which defeats the purpose. I think a solution would be to "ship" a tiny BIDS dataset as part of the test directory that we can use to actually execute the examples.

WDYT @hoechenberger @agramfort ?

agramfort · 2021-07-07T11:34:33Z

how would you "ship" it?

…

sappelhoff · 2021-07-07T11:40:57Z

how would you "ship" it?

just putting it there ... like the data in MNE-Python's IO module
as a git submodule ... so that getting it is optional (unless you want to run tests)
... do you have another idea?

agramfort · 2021-07-07T14:03:37Z

if we manage to make it less than 30MB I would say why not to add it to the git repo like mne/io/tests/data folder in mne-python

we did not do that before either, but --doctest-modules is now ON and that option apparently also runs the examples

hoechenberger · 2021-07-14T09:33:49Z

@sappelhoff Apparently you forgot one or two places? @ CI failing

sappelhoff · 2021-07-14T09:35:25Z

one is about bids_path.root, which can return a str OR a Path --> In this case it returns a Path, and on Windows this happens to be a WindowsPath, not posix. 🤷‍♂️

The other one, I am still investigating

hoechenberger · 2021-07-14T09:38:00Z

The other one, I am still investigating

I think it's because in the test you're comparing to a path constructed via os.path.join(), which will use \\ as separator by default on Windows.

hoechenberger · 2021-07-14T09:39:09Z

one is about bids_path.root, which can return a str OR a Path

Wait, why is that? IMHO it should always be a path …

sappelhoff · 2021-07-14T09:44:32Z

Wait, why is that? IMHO it should always be a path …

yes, also according to the attributes docs in the docstr. But see:

mne-bids/mne_bids/path.py

Lines 442 to 449 in 2d8a721

    
           @property 
        
           def root(self) -> Optional[Union[str, Path]]: 
        
               """The root directory of the BIDS dataset.""" 
        
               return self._root 
        
           @root.setter 
        
           def root(self, value): 
        
               self.update(root=value)

I am a little confused by that Union(str, Path)

sappelhoff · 2021-07-14T09:46:09Z

Maybe the solution is:

    def __repr__(self):
        """Representation in the style of `pathlib.Path`."""
        return f'{self.__class__.__name__}(\n' \
-              f'root: {self.root}\n' \
+              f'root: {self.root.as_posix()}\n' \
               f'datatype: {self.datatype}\n' \
               f'basename: {self.basename})'

but root can be "None" 🤔

sappelhoff · 2021-07-14T09:55:24Z

I think there is something I don't understand in

mne-bids/mne_bids/path.py

Lines 442 to 449 in 2d8a721

    
           @property 
        
           def root(self) -> Optional[Union[str, Path]]: 
        
               """The root directory of the BIDS dataset.""" 
        
               return self._root 
        
           @root.setter 
        
           def root(self, value): 
        
               self.update(root=value)

BIDSPath has an attribute "root" ... here defined with the @property decorator
the type hints show us it can return str or Path ... Is this an error? I think it should be Path and None ... never str
then it returns self._root, which is being set in the update method and is either None, or Path(val).expanduser(), where val is either Path or str
the setter is fine again: calling bids_path.root = Path("/home/bids_ds") (or with None, or a str) just calls "update" method again as in the step before

sappelhoff · 2021-07-14T09:58:30Z

Do we need a __str__ method for attributes? the bids_path.root attribute is just printed "as is" (a Path) 😕 ❓

hoechenberger · 2021-07-14T10:30:11Z

BIDSPath has an attribute "root" ... here defined with the @property decorator

the type hints show us it can return str or Path ... Is this an error? I think it should be Path and None ... never str

then it returns self._root, which is being set in the update method and is either None, or Path(val).expanduser(), where val is either Path or str

Considering point 3, I would agree that the type hint is incorrect and should be Optional[Path]

sappelhoff · 2021-07-14T10:32:12Z

Considering point 3, I would agree that the type hint is incorrect and should be Optional[Path]

mmh okay - but it could be None though. What do you think of 459c723 ?

BTW: the errors are now fixed. I circumvented the repr/str issue for bids_path attributes by calling .as_posix on them in the example. I feel like further improving this should be a future PR (I am exhausted with this one)

hoechenberger · 2021-07-14T10:32:40Z

Do we need a __str__ method for attributes? the bids_path.root attribute is just printed "as is" (a Path) 😕 ❓

we'd only need it for root, right? All other parts of the BIDSPath are strings or None anyway?
How would we do that? Would we need a new class _BIDSRoot or something?

hoechenberger · 2021-07-14T10:34:55Z

Considering point 3, I would agree that the type hint is incorrect and should be Optional[Path]

mmh okay - but it could be None though. What do you think of 459c723 ?

Optional[foo] is just shorthand for Union[foo, None].

In Python 3.10 we'll be able to simply write foo | None like in TypeScript, but … well, we're not quite there yet.

But your proposal in 459c723 is redundant; you can safely switch to Optional[Path] (more "idiomic") or Union[Path, None] (more explicit, but rather uncommon as far as I can tell)

sappelhoff · 2021-07-14T10:35:25Z

Thanks for the hint, will push a commit!

hoechenberger · 2021-07-14T10:36:23Z

Thanks for the hint

☝️ I see what you did there! 😎

hoechenberger · 2021-07-14T10:46:08Z

we'd only need it for root, right? All other parts of the BIDSPath are strings or None anyway?
How would we do that? Would we need a new class _BIDSRoot or something?

How about we always store BIDSPath.root as a string (str(Path(root).expanduser()) or something) and whenever we need it to concat a path, we just cat it to a Path again?

sappelhoff · 2021-07-14T10:51:38Z

How about we always store BIDSPath.root as a string (str(Path(root).expanduser()) or something) and whenever we need it to concat a path, we just cat it to a Path again?

Can we store it as a Path(root).as_posix() (posix str) and then later on call Path on that posix_str and it's work on Windows? If yes, that seems like a good way to go

hoechenberger · 2021-07-14T11:02:59Z

Can we store it as a Path(root).as_posix() (posix str) and then later on call Path on that posix_str and it's work on Windows? If yes, that seems like a good way to go

Nice idea, I think this could work

sappelhoff · 2021-07-14T11:18:53Z

Ok cool, I opened a new issue for it :-)

Please merge this one if you are happy @hoechenberger

I could then do the release next.

hoechenberger · 2021-07-14T11:20:49Z

Thanks a bunch, @sappelhoff!

fix doctests in path.py

60ed51e

sappelhoff commented Jul 6, 2021

View reviewed changes

mne_bids/path.py Outdated Show resolved Hide resolved

test: use pytest --doctest-modules

6f26c14

sappelhoff added 2 commits July 6, 2021 12:06

Merge branch 'main' into fix/doctests

a2c60c1

fix some more examples

0601d21

sappelhoff changed the title ~~[WIP] Fix doctests (and run in CI?)~~ [WIP] Add tiny BIDS test dataset, fix doctests, and run it in CI Jul 7, 2021

sappelhoff added the maint label Jul 7, 2021

sappelhoff added this to the 0.8 milestone Jul 8, 2021

sappelhoff mentioned this pull request Jul 9, 2021

Release 0.8 ? #816

Closed

sappelhoff added 8 commits July 13, 2021 16:55

Merge branch 'main' into fix/doctests

0b56bc3

make use of _write_json instead of json.dump

57d5565

fix fine-calib and crosstalk doctest

d3af6dd

DATA: add tiny_bids test dataset ~800kb

9bc1d1e

fix all doctests using new test ds

396d9bf

add code for generating tiny_bids

96fda84

fix example, sphinx warning

b5555f1

use tiny_bids in bidspath example, fix mne-tools#829

0067d11

sappelhoff marked this pull request as ready for review July 13, 2021 18:25

sappelhoff changed the title ~~[WIP] Add tiny BIDS test dataset, fix doctests, and run it in CI~~ [MRG] Add tiny BIDS test dataset, fix doctests, and run it in CI Jul 13, 2021

sappelhoff added 4 commits July 13, 2021 20:42

write TSV files with newline character

ea8fe32

add missing newlines at end of files

c605b8f

do not run pytest on examples

508047c

we did not do that before either, but --doctest-modules is now ON and that option apparently also runs the examples

add whatsnew for TSV line end

c0ae22f

fix op.join -> Path

f86b5e5

sappelhoff added 2 commits July 14, 2021 12:02

root in __repr__ --> posix path or None

b6558f4

circumvent bids_path.root __str__

dd4588d

sappelhoff force-pushed the fix/doctests branch from 012b354 to dd4588d Compare July 14, 2021 10:09

1) fix pep, 2) fix type hints, 3) fix example ds

459c723

fix type hint

6ab7a5e

I should run make pep before pushing

d09a1c9

sappelhoff mentioned this pull request Jul 14, 2021

storing BIDSPath.root attribute as a string instead of Path? #843

Open

hoechenberger approved these changes Jul 14, 2021

View reviewed changes

hoechenberger merged commit 0db039f into mne-tools:main Jul 14, 2021

sappelhoff deleted the fix/doctests branch July 14, 2021 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Add tiny BIDS test dataset, fix doctests, and run it in CI #831

[MRG] Add tiny BIDS test dataset, fix doctests, and run it in CI #831

sappelhoff commented Jul 6, 2021 •

edited

Loading

agramfort commented Jul 6, 2021

sappelhoff commented Jul 6, 2021

codecov-commenter commented Jul 7, 2021 •

edited

Loading

sappelhoff commented Jul 7, 2021

agramfort commented Jul 7, 2021 via email

sappelhoff commented Jul 7, 2021

agramfort commented Jul 7, 2021 via email

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

sappelhoff commented Jul 14, 2021 •

edited

Loading

sappelhoff commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

[MRG] Add tiny BIDS test dataset, fix doctests, and run it in CI #831

[MRG] Add tiny BIDS test dataset, fix doctests, and run it in CI #831

Conversation

sappelhoff commented Jul 6, 2021 • edited Loading

Merge checklist

agramfort commented Jul 6, 2021

sappelhoff commented Jul 6, 2021

codecov-commenter commented Jul 7, 2021 • edited Loading

Codecov Report

sappelhoff commented Jul 7, 2021

agramfort commented Jul 7, 2021 via email

sappelhoff commented Jul 7, 2021

agramfort commented Jul 7, 2021 via email

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

sappelhoff commented Jul 14, 2021 • edited Loading

sappelhoff commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 14, 2021

hoechenberger commented Jul 14, 2021

sappelhoff commented Jul 6, 2021 •

edited

Loading

codecov-commenter commented Jul 7, 2021 •

edited

Loading

sappelhoff commented Jul 14, 2021 •

edited

Loading