Skip to content
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

[WIP] Create adni json from xml metadata #431

Closed

Conversation

omar-rifai
Copy link
Contributor

Module to allow integration of xml metadata to the adni-to-bids clinical data converter

@pep8speaks
Copy link

pep8speaks commented Sep 23, 2021

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

Line 580:23: W605 invalid escape sequence '\d'

Comment last updated at 2022-02-22 09:10:05 UTC

@omar-rifai
Copy link
Contributor Author

Hello @emaheux. Please let me know if this would satisfy the requirements for leaspy.
The few modifications I made over your code is:

  • Running the xml metadata extractor when the argument --xml_path is given
  • Removing the use of the multiprocessing library (in the objective of switching to a multithreading/asyncio library).

@omar-rifai
Copy link
Contributor Author

I have kept the resulting file with all the data extracted separate from the BIDs datastructure but could have the write function add the data next to the images if necessary.

@omar-rifai omar-rifai marked this pull request as ready for review September 28, 2021 13:46
@emaheux
Copy link
Contributor

emaheux commented Sep 28, 2021

Thank you Omar :)

I have kept the resulting file with all the data extracted separate from the BIDs datastructure but could have the write function add the data next to the images if necessary.

It would have been nice to have this metadata as part of the BIDS structure (metadata of scans tsv? - or at least in merged tsv BIDS.tsv?) but if it is not possible / too complicated it will be fair enough to load this extra json file!

@omar-rifai
Copy link
Contributor Author

omar-rifai commented Sep 28, 2021

Thank you Omar :)

I have kept the resulting file with all the data extracted separate from the BIDs datastructure but could have the write function add the data next to the images if necessary.

It would have been nice to have this metadata as part of the BIDS structure (metadata of scans tsv? - or at least in merged tsv BIDS.tsv?) but if it is not possible / too complicated it will be fair enough to load this extra json file!

It should be doable! I was just wondering what would be best. I'll keep you posted.
(for merge tsv it should appear automatically if added to the scan tsv file).

@omar-rifai omar-rifai changed the title Create adni json from xml metadata [WIP] Create adni json from xml metadata Dec 3, 2021
@omar-rifai omar-rifai closed this Dec 10, 2021
@omar-rifai
Copy link
Contributor Author

Closed as no consensus found currently as to where this data should ideally be integrated. Merge-tsv should also see a refactoring soon (c.f issue 559) which goes against integrating the data there.

@omar-rifai omar-rifai reopened this Feb 22, 2022
@omar-rifai
Copy link
Contributor Author

omar-rifai commented Feb 22, 2022

Hi @emaheux, since @NicolasGensollen is working on similar topics, we were wondering if this is still relevant. I did not have the time to pursue the task but it can be an opportunity to pick up the work. Let us know.

@emaheux
Copy link
Contributor

emaheux commented Feb 22, 2022

Indeed @omar-rifai, this is still relevant (though obviously not blocking / top-priority since we circumvented the issue long time ago by adding ADNI imaging metadata downstream by ourselves with the codebase I gave you).

Yet if a Leaspyer needs to conduct experiments with a newer version of ADNI, I think it would be really nice / more consistent to have those imaging metadata directly accessible in Clinica outputs!

@omar-rifai omar-rifai closed this Mar 25, 2022
@omar-rifai omar-rifai deleted the feat_add_adni_json_metadata branch March 31, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants