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

Add dianne tutorials #734

Merged
merged 19 commits into from
Feb 12, 2024
Merged

Add dianne tutorials #734

merged 19 commits into from
Feb 12, 2024

Conversation

asmacdo
Copy link
Member

@asmacdo asmacdo commented Jan 18, 2024

This PR adds some tutorials from Dianne Patterson at University of Arizona: https://neuroimaging-core-docs.readthedocs.io/en/latest/index.html (with permission, thanks Dianne!)

I've moved some things around so the left bar didn't get so cluttered.

Screenshot from 2024-01-18 12-12-57

Remaining TODOs:

  • Fix links to outside docs
  • Example Dataset
  • Add Podman option to install
  • Use heudiconv directly, without container in the tutorials
  • Add Docker/Podman example commands to CLI reference
  • convert hdc_run.sh to docs

Originally written by Dianne Patterson for the U of A group, these
tutorials are generally useful to all heudiconv users.

This commit brings those tutorials verbatim and will be edited in
subsequent commits.

    Co-authored-by: Dianne Patterson <dkp@arizona.edu>
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (81f0d2f) 81.97% compared to head (e0f9ce2) 81.97%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #734   +/-   ##
=======================================
  Coverage   81.97%   81.97%           
=======================================
  Files          41       41           
  Lines        4149     4149           
=======================================
  Hits         3401     3401           
  Misses        748      748           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheChymera
Copy link
Contributor

Looks good to me, other than failing lint/spell.

One thing though, I don't think “Heuristic File Reference” makes it any clearer.
If anything it's more confusing....

As per the discussion on Monday, you did mention heuristic is a bit vague.
I would say — and it might be some professional jargon I picked up — that “heuristic” is a pretty clear concept in parsing/reformatting things.
If you feel clarification is needed, perhaps “Heuristic Specification File” would be clearer, i.e. the file which specifies the heuristic.

@yarikoptic
Copy link
Member

I would not abuse "specification" word here: if used explicitly it usually refers to some formalized semantic description (e.g. recipe as amounts of each incredient, not really the cooking instructions per se). Heuristic ATM is Python code to provide desired organization (directories) and names to the files given DICOMs metadata.

@yarikoptic
Copy link
Member

FWIW -- pushed 2 little commits to clarify provenance of the docs. Did Dianne state the license?

@asmacdo
Copy link
Member Author

asmacdo commented Jan 18, 2024

No but Dianne gave me permission to use her materials upstream.

docs/quickstart.rst Outdated Show resolved Hide resolved
@asmacdo
Copy link
Member Author

asmacdo commented Jan 22, 2024

Goal: make the example dataset minimal (delete extra files)

I tried deleting all sequences except DTI_30_DIRs_AP_15, but it failed

    Traceback (most recent call last):
      File "/home/austin/miniconda3/bin/heudiconv", line 8, in <module>
        sys.exit(main())
                 ^^^^^^
      File "/home/austin/miniconda3/lib/python3.11/site-packages/heudiconv/cli/run.py", line 30, in main
        workflow(**kwargs)
      File "/home/austin/miniconda3/lib/python3.11/site-packages/heudiconv/main.py", line 479, in workflow
        prep_conversion(
      File "/home/austin/miniconda3/lib/python3.11/site-packages/heudiconv/convert.py", line 237, in prep_conversion
        info = heuristic.infotodict(seqinfo_list)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/austin/datasets/sub-219_dicom/MRIS/Nifti/code/heuristic1.py", line 106, in infotodict
        raise ValueError('\n'.join(msg))
    ValueError: WARNING: Missing correct number of T1w runs
    WARNING: Missing correct number of RPE runs
    WARNING: Missing correct number of magnitude runs
    WARNING: Missing correct number of phasediff runs
    WARNING: Missing correct number of resting_state runs
    WARNING: Missing correct number of resting_state_post runs

@yarikoptic
Copy link
Member

I guess you removed too much -- that is the code/logic in their heuristic. Check which files they care about there.

@asmacdo asmacdo force-pushed the add-dianne-tutorials branch from 40deea6 to 2c5dfff Compare January 31, 2024 20:09
@asmacdo asmacdo force-pushed the add-dianne-tutorials branch from 2c5dfff to 5de5e71 Compare January 31, 2024 20:13
Copy link
Member Author

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the tutorial still makes some assumptions about user-knowledge of MRIs BIDs, which might be just fine for this project-- I just need someone more familiar to review.

docs/reproin.rst Outdated Show resolved Hide resolved

Reproin Scanner File Names
****************************

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yarikoptic This section needs review from someone who is more familiar with bids/reproin/mri

@dkp
Copy link

dkp commented Jan 31, 2024

I'm happy to give back to the community. Attribution is lovely (and makes my bosses happier).

@asmacdo asmacdo marked this pull request as ready for review February 1, 2024 18:30
@yarikoptic
Copy link
Member

yarikoptic commented Feb 3, 2024

don't know yet why RTD does not report back on builds, although it does them for PRs e.g. https://readthedocs.org/projects/heudiconv/builds/23301131/ . I have checked oauth permissions on personal and nipy org levels -- all seems good. I found that there were 2 instead of one webhooks for github , removed one and resynced another. Let's see if may be it would emerge back

edit: actually for this PR 734 the last one from 2 weeks ago. The most recent one was for #735 from a day ago and there it reported fine. So something must be special about this PR :-/

LICENSE Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
NeuroStars.org is a platform similar to StackOverflow but dedicated to neuroinformatics.

All previous ``heudiconv`` questions are available here:
http://neurostars.org/tags/heudiconv/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good, again independent of the goal of the PR

docs/container.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
@asmacdo
Copy link
Member Author

asmacdo commented Feb 3, 2024

pushed your suggested license commit to see if that kicks off a build

docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated

**TODO --- Is this generally useful or should be cut???**

Here is this phantom dataset displayed in the scanner dot cockpit. The directory structure is defined at the top: *Patterson >> Coben >> Patient*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Says "Here" like there would be an image like those https://github.com/ReproNim/reproin/blob/master/docs/walkthrough-1.md#new-program
anything missing?

docs/reproin.rst Outdated Show resolved Hide resolved
docs/reproin.rst Outdated Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@asmacdo asmacdo closed this Feb 9, 2024
@asmacdo asmacdo reopened this Feb 9, 2024
@asmacdo asmacdo merged commit 0e92ac3 into nipy:master Feb 12, 2024
9 checks passed
@yarikoptic yarikoptic added the documentation Changes only affect the documentation label Feb 24, 2024
Copy link

🚀 PR was released in v1.0.2 🚀

Copy link

🚀 PR was released in v1.0.2 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Changes only affect the documentation released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants