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

[FIX] Show logs in re-runs #637

Merged
merged 6 commits into from
Jan 4, 2021
Merged

[FIX] Show logs in re-runs #637

merged 6 commits into from
Jan 4, 2021

Conversation

notZaki
Copy link
Contributor

@notZaki notZaki commented Dec 29, 2020

Closes #606.

Changes proposed in this pull request:

  • Instead of removing all handlers from logging.root at the end of the worklow, only remove sh and log_handler
    • The root logger contains other handlers if pytest is being used, and one of those handlers is responsible for printing to the terminal, so we don't want to remove these handlers.
  • Manually add handlers to logging.root instead of using logging.basicConfig()
    • basicConfig does nothing if logging.root already has handlers
      • logging.root will have no handlers if a user is calling tedana, but it does have handlers if pytest is being used
    • In the current tests, basicConfig does nothing in the first run because logging.root already has handlers, but then these handlers are deleted at the end of the first run. On the second run, basicConfig works properly.
      • This is probably why the first run doesn't produce any tedana_$DATE.tsv file in the output directory (or it is created, but file is empty)
  • Close and remove the handlers for RefLGR and RepLGR at the end of workflow
    • Otherwise, the logger keeps printing to the old file when re-running, e.g. report.txt from the first run will contain appended text from the second run
  • Typo: One of the RepLGR should've been RefLGR
    • Previously, the references would only be included in report.txt if verbose mode was enabled (or maybe it had to be debug mode, forgot which one specifically)

Possible discussion points:

  • When calling tedana (not through pytest) the terminal printout used to look like:
INFO:tedana.workflows.tedana:Computing EPI mask from first echo
DEBUG:tedana.workflows.tedana:Retaining 38643/178752 samples
INFO:tedana.workflows.tedana:Computing T2* map
DEBUG:tedana.workflows.tedana:Setting cap on T2* map at 0.10164s
INFO:tedana.combine:Optimally combining data with voxel-wise T2* estimates
INFO:tedana.decomposition.pca:Computing PCA of optimally combined multi-echo data
INFO:tedana.decomposition.ma_pca:Performing SVD on original OC data...

but after this PR, it looks like

Computing EPI mask from first echo
Retaining 38643/178752 samples
Computing T2* map
Setting cap on T2* map at 0.10164s
Optimally combining data with voxel-wise T2* estimates
Computing PCA of optimally combined multi-echo data
Performing SVD on original OC data...

The tedana_$DATE.tsv still looks like the former, so it remains unchanged.

@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #637 (a3ff341) into master (45e5ad7) will increase coverage by 0.02%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #637      +/-   ##
==========================================
+ Coverage   93.54%   93.56%   +0.02%     
==========================================
  Files          26       26              
  Lines        1967     1975       +8     
==========================================
+ Hits         1840     1848       +8     
  Misses        127      127              
Impacted Files Coverage Δ
tedana/workflows/tedana.py 90.40% <93.33%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45e5ad7...a3ff341. Read the comment docs.

Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I checked over the test logs and those look good as well. Thanks for this!

Comment on lines -378 to +377
RepLGR.setLevel(logging.INFO)
RefLGR.setLevel(logging.INFO)
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, maybe we should make those variables more distinct, or else we'll probably just do this again later.

@tsalo tsalo requested a review from jbteves December 30, 2020 19:19
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

These changes look good to me, approved! Thanks @notZaki

@tsalo
Copy link
Member

tsalo commented Jan 4, 2021

Thanks for reviewing @jbteves. Now that we have the necessary reviews, I'll merge.

@tsalo tsalo merged commit 5369e1d into ME-ICA:master Jan 4, 2021
@notZaki notZaki deleted the showlogs branch January 17, 2021 03:35
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.

Tests do not show logs from workflow _main() functions
3 participants