Skip to content

Comments

Add Datagrabber and cofound adaptation for cat processed HCP-Aging#2

Open
jkroell wants to merge 15 commits intojuaml:mainfrom
jkroell:main
Open

Add Datagrabber and cofound adaptation for cat processed HCP-Aging#2
jkroell wants to merge 15 commits intojuaml:mainfrom
jkroell:main

Conversation

@jkroell
Copy link

@jkroell jkroell commented Oct 23, 2024

No description provided.

Copy link
Collaborator

@LeSasse LeSasse left a comment

Choose a reason for hiding this comment

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

i think maybe put it into this existing file https://github.com/juaml/juni-farm/blob/main/juni_farm/datagrabber/hcp_aging.py

then everything for hcp aging is in there and no need to then copy the existing hcp aging datagrabber.

and lastly you can then make the multiple aging datagrabber similar to https://github.com/juaml/juni-farm/blob/main/juni_farm/datagrabber/hcp_ya_confounds_cat.py#L284

dont forget to update the description and add yourself as an author

@@ -0,0 +1,409 @@
"""Provide a datagrabber for CAT-processed confounds in the HCP dataset."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update the description?

Copy link
Member

Choose a reason for hiding this comment

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

Are you still waiting for an update or shall is it resolved?

Copy link
Collaborator

@LeSasse LeSasse Oct 25, 2024

Choose a reason for hiding this comment

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

still needs an update i think @jkroell i think this file needs to be removed

Copy link
Member

Choose a reason for hiding this comment

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

The naming is a bit awkward with the "Aging" part, maybe make it "aging"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this file actually needs to be removed, everything is in hcp_aging.py @jkroell

"""Provide a datagrabber for the HCP Aging dataset and its confounds."""

# Authors: Leonard Sasse <l.sasse@fz-juelich.de>
# Authors: Leonard Sasse <l.sasse@fz-juelich.de>, Jean-Philippe Kroell <j.kroell@fz-juelich.de>
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be in two separate lines like so:

# Authors: Leonard Sasse <l.sasse@fz-juelich.de>
#          Jean-Philippe Kroell <j.kroell@fz-juelich.de>

Returns
-------
dict
keys (CAT variables) and values (corresponding fMRIprep variables).
Copy link
Member

Choose a reason for hiding this comment

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

Cosmetic nitpick: Keys (CAT variables) and values (corresponding fMRIprep variables).

Parameters
----------
datadir : str or Path, optional
The directory where the datalad dataset will be cloned.
Copy link
Member

Choose a reason for hiding this comment

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

(default None). should be the ending for it. Before that, it'd be good to add what happens in the default case like so:
... . By default, <it-does-magic> (default None).

Copy link
Member

Choose a reason for hiding this comment

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

Might need to go through ruff for formatting and linting.

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