Skip to content

Make datman easier to install and use#303

Merged
DESm1th merged 35 commits intoTIGRLab:masterfrom
DESm1th:easybake_datman
Apr 25, 2022
Merged

Make datman easier to install and use#303
DESm1th merged 35 commits intoTIGRLab:masterfrom
DESm1th:easybake_datman

Conversation

@DESm1th
Copy link
Copy Markdown
Contributor

@DESm1th DESm1th commented Apr 30, 2021

This pull request adds:

  • Fixes for some bugs that arise when running without a dashboard
  • Well commented configuration templates in assets/config_templates that can be used by anyone who doesnt have access to our datman-config repo
  • A dockerfile documenting how tigrlab/datman:0.1 was built
  • Updates and fixes to our documentation pages on how to install and configure datman

Due to the fact that datman.utils.update_checklist was expecting
subject IDs without the repeat/session number, these scripts were
causing exceptions to be raised on checklist update when the
dashboard wasnt installed.
- Started to remove hard-coded redcap config
- Updated documentation on configuration and installation
@auto-assign auto-assign bot requested review from edickie and jerdra April 30, 2021 16:59
@auto-assign auto-assign bot requested a review from josephmje April 30, 2021 16:59
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Apr 30, 2021

Hello @DESm1th, 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 2022-02-23 19:46:55 UTC

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 this @DESm1th! Especially all of that reformatting of the docs. I've yet to go over the config files but had 2 minor comments to start.

Comment thread Dockerfile Outdated
Comment thread assets/config_templates/main_config.yml Outdated
DESm1th and others added 2 commits May 4, 2021 11:51
Co-authored-by: Michael Joseph <josephmje.22@gmail.com>
Co-authored-by: Michael Joseph <josephmje.22@gmail.com>
@DESm1th
Copy link
Copy Markdown
Contributor Author

DESm1th commented May 4, 2021

Thanks for the feedback Mike! :)

josephmje
josephmje previously approved these changes May 4, 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.

Thanks for putting this together Dawn! Just had a few verrry minor items that could maybe be addressed in a separate PR but i thought would be worth mentioning!

Comment thread README.rst Outdated
Comment thread bin/dm_link_sprl.py
Comment on lines 52 to 54
ch = logging.StreamHandler(sys.stdout)
ch.setLevel(logging.WARN)
logger.setLevel(logging.WARN)
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.

Is there a reason to have varying logging set-ups across scripts?

Copy link
Copy Markdown
Contributor Author

@DESm1th DESm1th Oct 5, 2021

Choose a reason for hiding this comment

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

Nope, none at all (except in places where we add a server log handler). This is just a historical thing leftover from the days when we joe/tom/I had three different ideas about how it should be done. I've always meant to clean up these old instances where basicConfig wasnt used though

Comment on lines +156 to +163
def get_setting(key, default=None):
try:
return cfg.get_key(key)
except datman.config.UndefinedSetting as e:
if not default:
raise e
return default

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 get_setting be defined in the Config object instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think there was a good reason at the time why I put it here and not in config.py, but I can't currently remember what that was. So maybe I should move it over? I'll try to job my memory on this one

Co-authored-by: Jerry Jeyachandra <jerrold.jeyachandra@gmail.com>
@josephmje
Copy link
Copy Markdown
Contributor

Sorry about the delay on this @DESm1th! Is this now ready for review?

@DESm1th
Copy link
Copy Markdown
Contributor Author

DESm1th commented Jan 4, 2022

Sorry about the delay on this @DESm1th! Is this now ready for review?

It is!

Comment thread Dockerfile Outdated
ln -s pip3 pip && \
pip install --upgrade pip

# Install dcm2niix/v1.0.20210317
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.

Could we upgrade the version of dcm2niix here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! Thanks for catching that. What version should I add here? Just the newest?

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.

Hey Dawn, sorry for the late reply. Yes, it would be v1.0.20211006

@jerdra Do you know if this latest version is correct or the 2019 version RE the OPT slice timing?

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.

its probably fine given that we don't use slice-timing for fMRI scans anyway. The diffusion scans aren't affected

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok! I updated the version used

@DESm1th DESm1th merged commit 6ac4827 into TIGRLab:master Apr 25, 2022
@DESm1th DESm1th deleted the easybake_datman branch April 25, 2022 17:48
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.

4 participants