[WIP] Parse ADNI XML metadata

[NicolasGensollen]

This PR continues the work started in #431 for adding metadata from XML files to the output of the ADNI to BIDS convertor. (Note: I made a lot of refactoring so I went for opening a new PR since the diff would be huge anyway...).

• Refactor adni_json.py: shorter functions (especially parse_xml_file), more explicit variable and function names, docstrings, all functions private except create_json_metadata...
• Change output column names to be lower case (seems to better match BIDS specs for tabular files)
• Add JSON file to better describe column names ?
• Add tests

[pep8speaks]

Hello @NicolasGensollen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-03-30 08:20:43 UTC

[ghisvail]

Back when the first PR was proposed, I was wondering whether we could leverage the new pandas.read_xml to parse the XML metadata instead of raw xml.etree manipulations.

My thinking being: could we just let pandas do the actual XML parsing and only maintain code that selects and combines the metadata attributes we want? With the hypothesis that this would greatly simplify the resulting code and the future review of this PR.

[omar-rifai]

Thanks @NicolasGensollen for picking this up. Also, for the record, the two main elements that were blocking for merging 431 were (in order of priority):

• Defining which metadata to include (c.f discussion#293) and where they should be written in the BIDS hierarchy
• Eventually parallelizing with an appropriate library (not a priority)

[omar-rifai]

@NicolasGensollen, before finalizing can you make sure to add @emaheux to the author co-author of the first commit as he provided the draft code for this. Thanks!

[NicolasGensollen]

@omar-rifai Of course ! I can also add you as the author of another one since I started from your work!

[omar-rifai]

Hello @NicolasGensollen, let us know if this is finalized so that we can review and merge. Thanks !

[NicolasGensollen]

Hi @omar-rifai
I believe this is ready for reviews.
I re-ran the converter this afternoon (see folder data_ci_2022_xml, the output is in ref).

[omar-rifai]

Looks good thanks ! I'll merge after the rebase.

[omar-rifai]

what is the rationale behind this comment? Threading seems to be designed for that purpose.

[NicolasGensollen]

As discussed, we can remove the comment

[NicolasGensollen]

Done in d8b7d59

[omar-rifai]

Probably related to the test itself but I'm getting an TypeError on test_get_existing_scan_dataframe:

 def test_get_existing_scan_dataframe(tmp_path):
        """Test function `_get_existing_scan_dataframe`."""
        from clinica.iotools.converters.adni_to_bids.adni_json import _get_existing_scan_dataframe
        from pandas.testing import assert_frame_equal
        subj_path = tmp_path / "sub-01"
        subj_path.mkdir()
>       with pytest.warns(match="No scan tsv file for subject sub-01 and session foo"):
E       TypeError: warns() missing 1 required positional argument: 'expected_warning'

[omar-rifai]

@NicolasGensollen, can you also please run a make format (again?) so that it passes the lint checks?

[NicolasGensollen]

can you also please run a make format (again?)

Done in 6034162

I'm getting an TypeError on test_get_existing_scan_dataframe

Oops, missed your comment on Friday.
Did you manage to run the tests since then?
Doing pytest -v works on my end.

[omar-rifai]

can you also please run a make format (again?)

Done in 6034162

I'm getting an TypeError on test_get_existing_scan_dataframe

Oops, missed your comment on Friday. Did you manage to run the tests since then? Doing pytest -v works on my end.

Yes seems to work now, thanks! I also fixed some issues with the data_ci and the converters succeed now. However, It seems that there is still an issue with merge_tsv in iotools. Can you check that is everything merged ? I had a similar issue last week from a recent change in dev.