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

[WIP] Fix and uniformize verbose #900

Closed
wants to merge 4 commits into from

Conversation

karanphil
Copy link
Contributor

@karanphil karanphil commented Feb 14, 2024

Quick description

I tried to uniformize the verbose option across scripts. This was a nightmare. It is a really big WIP. I will need input from power users of each scripts. For example:

I am uncertain of all bundle/connectivity scripts. Everything that loads or saves tractograms has logging.info somewhere (formely logging.debug), so I added a verbose option for INFO. But there might be some debug in Dipy... The tractogram scripts probably all have the same issue, so I did touch them for the moment.

Also, I changed the level from DEBUG to INFO in scil_freewater_maps.py and scil_NODDI_maps.py, but I am not sure of the logging level in COMMIT.

Some cases are hard to choose between DEBUG and INFO... like scil_gradients_generate_sampling.py. There is a reason for both INFO and DEBUG to be present, but now that WARNING is the default, we only have the option to see none of the messages, or all of it (both INFO and DEBUG), or never see the DEBUG stuff. scil_NODDI_priors.py also have this problem... both debug and info, so with WARNING as default we don't have a choice between INFO and DEBUG with verbose. Same for scil_search_keywords.py.

I am not sure I understand why the verbose argument is setup the way it is in scil_tracking_pft.py.

I did not touch the visualization scripts.

...

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

...

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@karanphil karanphil closed this Feb 15, 2024
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.

1 participant