bbfrederick / rapidtide

rapidtide - a suite of programs for doing time lag correlation analysis on fMRI data
Apache License 2.0
75 stars 14 forks source link

[REF] Continue shifting to logging #68

Closed tsalo closed 3 years ago

tsalo commented 3 years ago

References #41.

Changes proposed in this pull request:

tsalo commented 3 years ago
  1. I worry a little about the execution time impact of the debugging logger - some of the debugging output can be kind of lengthy; in the existing system, that code is never executed if debugging is off. It appears that now that code is always run, it's just a question of whether you look at the debug logs or not. Also, I don't know how big the debugging logs could potentially get, if they are run all the time. Finally, some of my debugging code does things like throw up plots - not sure how that is handled.

I hadn't really thought about the time costs for those cases. I'm sure there's a way to inspect the LGR to determine its logging level, and then use that in an if statement for those cases where the debug messages involve running new code. I can look into it.

The debugging logs only print to the command line and the log file if the logging level is set to include debug messages. In cases where users use --debug, I imagine both could get quite long, but I think that was always the case, right?

The plot thing is also something I didn't consider, but I think the solution can be the same as the first issue (i.e., check the logger's logging level and then toggle plot creation based on that).

  1. Some routines, such as dlfilter, are called by happy, not rapidtide. On the last round of changes I had to port over some of the logger initialization stuff because of crashes in routines called by happy - since I don't see any additions to rapidtide in this round, there may be no new issues, but if you can think of any, let me know.

Sorry about that! I didn't even think of accounting for happy. I was just planning to work through the whole package file by file. I can keep an eye out for problems, though, and run some tests with happy as well as rapid tide.

tsalo commented 3 years ago

LGR.getEffectiveLevel() looks like what we want to check the level in the code. I will add something like if LGR.getEffectiveLevel() <= logging.DEBUG for those cases where the debugging messages involve extra calculations or plots.

bbfrederick commented 3 years ago

Sounds like a plan.

bbfrederick commented 3 years ago

Added in a new routine, test_smoke.py, that confirms that happy and rapidtide run, which should catch any breaking changes early. This required some rejiggering of the argument parsing, but it seemed to be compatible with your PR.