Skip to content

[ENH] Add dm_parse_faces#307

Merged
jerdra merged 5 commits intoTIGRLab:masterfrom
jerdra:feature/dm_parse_faces
Jun 17, 2021
Merged

[ENH] Add dm_parse_faces#307
jerdra merged 5 commits intoTIGRLab:masterfrom
jerdra:feature/dm_parse_faces

Conversation

@jerdra
Copy link
Copy Markdown
Contributor

@jerdra jerdra commented Jun 16, 2021

Fixed up PEP8 issues and added a bit more clarity to logging

@auto-assign auto-assign bot requested review from DESm1th, benselby and jskocic June 16, 2021 17:52
@jerdra jerdra requested review from josephmje and kyjimmy and removed request for benselby and jskocic June 16, 2021 17:52
Comment thread bin/dm_parse_faces.py
Comment thread bin/dm_parse_faces.py
Copy link
Copy Markdown
Contributor

@kyjimmy kyjimmy left a comment

Choose a reason for hiding this comment

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

Variables name in the output text file of TAY project is different from other other projects.

kyjimmy
kyjimmy previously approved these changes Jun 16, 2021
Copy link
Copy Markdown
Contributor

@kyjimmy kyjimmy left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you @jerdra for putting everything together! :)

Comment thread bin/dm_parse_faces.py

log_head, log_tail = os.path.split(eprimefile)

if not out_dir:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

--output-dir OUT_DIR Specify an alternate output directory [Default: $DATMAN_PROJECTSDIR/{study}/data/nii/{session}]

Wouldn't out_dir never be empty since there's a default value? Please correct me if I am wrong.

Copy link
Copy Markdown
Contributor Author

@jerdra jerdra Jun 17, 2021

Choose a reason for hiding this comment

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

out_dir would be empty if the --output-dir arg isn't specified. The default in the docstring is just so the user knows it'll be put into the configured archive folder, it won't actually set a value (docopt will silently fail to make it a real value).

josephmje
josephmje previously approved these changes Jun 17, 2021
Copy link
Copy Markdown
Contributor

@josephmje josephmje left a comment

Choose a reason for hiding this comment

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

Thanks @jerdra! Just left some minor comments.

Comment thread bin/dm_parse_faces.py Outdated

Usage:
dm_parse_faces.py [options] <study>
dm_parse_faces.py [options] <study> [<session>]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this need to take multiple sessions? Can add [<session>]...

Comment thread bin/dm_parse_faces.py Outdated
nii_path, ident.get_full_subjectid_with_timepoint())

file_name = os.path.join(out_dir, f"{ses}_FACES.tsv")
os.makedirs(os.path.dirname(file_name), exist_ok=True)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this bit be moved to within the if not dryrun statement?

@jerdra jerdra dismissed stale reviews from josephmje and kyjimmy via b833f8b June 17, 2021 15:24
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Jun 17, 2021

Hello @jerdra, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 datman.

Comment last updated at 2021-06-17 15:29:25 UTC

@jerdra jerdra merged commit 7027b36 into TIGRLab:master Jun 17, 2021
@jerdra jerdra deleted the feature/dm_parse_faces branch June 17, 2021 15:41
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.

5 participants