pixell icon indicating copy to clipboard operation
pixell copied to clipboard

Change print statements to `logging.info`

Open msyriac opened this issue 5 years ago • 6 comments

e.g. the print statements in pixell.lensing.

This will allow the user to redirect all output in their program to their desired log file (or by default print to the console as usual).

msyriac avatar Nov 10 '20 16:11 msyriac

I might go ahead and do this repo-wide -- any objections @amaurea @mhasself ?

msyriac avatar Nov 10 '20 16:11 msyriac

Seems like a good move to me.

mhasself avatar Nov 10 '20 16:11 mhasself

Hmm the default logging level is "warning", not info, so just replacing print with logging.info will make all the printed stuff disappear. This makes this not a very good solution.

msyriac avatar Nov 10 '20 20:11 msyriac

To make this work, we'd also need to have this line:

logging.getLogger().setLevel(logging.INFO)

at the top of any module that uses logging. Not very elegant.

msyriac avatar Nov 10 '20 23:11 msyriac

Apologies if my earlier "seems like a good move" implied that I knew enough about logging to think it could work... I am new to logging but have been meaning to read up on it more. Now that I have played with it a bit:

  • Note that you could easily wrap that import logging; logging.getLogger().setLevel(logging.INFO) call, which is yucky boilerplate for someone who wants pixell lensing to be verbose, into a function pixell.lensing.be_verbose(). However, I don't think you should do that. Rather...
  • To support advanced logging users, don't use the root logger. Instead, get a per-module logger, because then a user can increase verbosity on a target module as needed.
  • Printing things to the screen, by default, using the logging module, is not really supported. There are ways to do it but they will all frustrate the "advanced" logging users mentioned in last point. Instead, I think your "if verbose: print(...)" is right.
  • In your draft PR, you replaced a bunch of print(...) with logger.info(...), even if they were already protected by "if verbose:". I think the ideas behind logging work best if you always call logger.info(...). In the case that your function has a "verbose" switch, then in addition to logger.info() you should print to screen. For example, in lensing.py consider initializing:
import logging
logger = logging.getLogger('pixell.lensing')

def vinfo(verbose, msg):
    logger.info(msg)
    if verbose:
        print(msg)

Then in each verbose-enabled function you would log info messages with vinfo(verbose, "Lensing activity L7H is happening now."). (If you find this pattern proliferating throughout pixell then of course it should be put into utils, perhaps like:

# In utils.py
def get_logger(module_name):
    logger = logging.getLogger(module_name)
    def vinfo(verbose, msg):
        logger.info(msg)
        if verbose:
            print(msg)
    return logger, vinfo

# in lensing.py
logger, vinfo = utils.get_logger('pixell.lensing')

) ((There's an additional encapsulation where vinfo is actually a member function of a new subclass of Logger; that's the most OOP solution here... I haven't quite read enough to know that that would work right but it might.))

mhasself avatar Nov 11 '20 13:11 mhasself

I like this suggestion, thanks a lot Matthew! I'll update my PR with that and play with it a bit.

msyriac avatar Nov 23 '20 21:11 msyriac