Skip to content

Update dm_mripicture.py#304

Merged
josephmje merged 3 commits intoTIGRLab:masterfrom
kyjimmy:master
Jun 17, 2021
Merged

Update dm_mripicture.py#304
josephmje merged 3 commits intoTIGRLab:masterfrom
kyjimmy:master

Conversation

@kyjimmy
Copy link
Copy Markdown
Contributor

@kyjimmy kyjimmy commented Jun 4, 2021

add logging module and skip subjects with existing files

add logging module and skip subjects with existing files
@auto-assign auto-assign bot requested review from jerdra, josephmje and jskocic June 4, 2021 19:23
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 for the update @kyjimmy! I just have some minor suggestions about using f strings instead of format for readability and using swapping --replace with --force.

Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
outdir = os.path.join(config.get_path('data'), 'tshirt')

os.makedirs(outdir, exist_ok=True)
logger.debug('Output location set to: {}'.format(outdir))
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.

Suggested change
logger.debug('Output location set to: {}'.format(outdir))
logger.debug(f'Output location set to: {outdir}')

Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
josephmje
josephmje previously approved these changes Jun 15, 2021
Copy link
Copy Markdown
Contributor

@jerdra jerdra left a comment

Choose a reason for hiding this comment

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

looks good! just a minor nitpick on --output

Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
Comment thread bin/dm_mripicture.py Outdated
@pep8speaks
Copy link
Copy Markdown

Hello @kyjimmy, Thank you for updating!

Line 15:81: E501 line too long (85 > 80 characters)

To test for issues locally, pip install flake8 and then run flake8 datman.

Copy link
Copy Markdown
Contributor

@jerdra jerdra left a comment

Choose a reason for hiding this comment

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

Good work!

@josephmje josephmje merged commit 57948d4 into TIGRLab:master Jun 17, 2021
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