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

Change print statements to logging.info #116

Open
msyriac opened this issue Nov 10, 2020 · 6 comments
Open

Change print statements to logging.info #116

msyriac opened this issue Nov 10, 2020 · 6 comments

Comments

@msyriac
Copy link
Member

msyriac commented Nov 10, 2020

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
Copy link
Member Author

msyriac commented Nov 10, 2020

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

@mhasself
Copy link
Member

Seems like a good move to me.

@msyriac msyriac mentioned this issue Nov 10, 2020
@msyriac
Copy link
Member Author

msyriac commented Nov 10, 2020

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
Copy link
Member Author

msyriac commented Nov 10, 2020

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.

@mhasself
Copy link
Member

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.))

@msyriac
Copy link
Member Author

msyriac commented Nov 23, 2020

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

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 a pull request may close this issue.

2 participants