cositools / cosipy

The COSI high-level data analysis tools
Apache License 2.0
3 stars 16 forks source link

Change print() to logging.info() #48

Open israelmcmc opened 1 year ago

israelmcmc commented 1 year ago

When thing's settle, we'll have to go through the code and find these out.

israelmcmc commented 2 months ago

Remaining offenders:

./data_io/UnBinnedData.py
./data_io/ReadTraTest.py
./data_io/BinnedData.py
./ts_map/fast_ts_fit.py
./ts_map/TSMap.py
./threeml/COSILike.py
./response/FullDetectorResponse.py
./image_deconvolution/RichardsonLucy.py
./image_deconvolution/data_loader.py
./image_deconvolution/exposure_table.py
./image_deconvolution/deconvolution_algorithm_base.py
./image_deconvolution/image_deconvolution.py
./spacecraftfile/SpacecraftFile.py

Tagging POCs for awareness:

This is a very "good first issue", so try to delegate!

ckarwin commented 1 month ago

@krishnatejavedula Do you know if this is still an issue for any of the other modules? I think maybe ts_map and image_deconvolution?

israelmcmc commented 1 month ago

Yeah, good catch @ckarwin. Some of the files I listed above were not in the changed files from #178, so it's likely that they are still an issue.

krishnatejavedula commented 1 month ago

Hello @israelmcmc, @ckarwin. You are right. It is still in an issue for the rest of the files and I have yet to make the changes for them. I will be doing that soon and include them in my next PR.

hiyoneda commented 1 month ago

@israelmcmc @ckarwin, do you plan to use logging.info as an output for any data analysis? I understand that we should replace 'print()' with logging.info(), warning(), etc. for debugging and logging. However, we sometimes want to show the results in the data analysis process, e.g., the number of extracted events, the significance of the detection, the total flux, etc. Since they are not the logging info, can we still use print or pprint for them?

ckarwin commented 1 month ago

@israelmcmc should comment as well, but I'll give my input. You can also take a look at the related discussion in the comments of PR #178. My general understanding is that logging replaces all print statements. Then anything that would normally be printed to terminal should be logged with logging.info(). The user can then set the logging configuration as desired. Note that they only need to configure the root logger, and all other modules will use the same. For example, they can set it as follows, in which case all the logging info will be written to file and printed to terminal:

logging.basicConfig(filename='cosipy.log', level=logging.INFO)
logging.getLogger().addHandler(logging.StreamHandler(sys.stdout))

I completely agree that we sometimes want to show the results in the data analysis process. I guess the general recommendation is for a library not to set the logger configuration (see discussion in PR #178). However, for the primary use case of cosipy, I think we need to have some terminal output to help inform the user about the flow of the analysis. This is specifically important for new users, students, etc., and they will likely not be considering that they need to set the logger configuration. Maybe we can have the configuration be part of the global parameter configurator in cosipy, where the default prints logging.info to terminal?

hiyoneda commented 1 month ago

Thank you, Chris. I missed the discussion in #178. OK, I will replace all of the print with logging.info. Yes, I agree that some information should be shown on a terminal by default for users. It may be better to set it somewhere. As for now, I will put this at the beginning of the notebook.

import logging
import sys
logger = logging.getLogger('cosipy')
logger.setLevel(logging.INFO)
logger.addHandler(logging.StreamHandler(sys.stdout))

Indeed, logging.basicConfig(filename='cosipy.log', level=logging.INFO) also affects the output from other modules.

ckarwin commented 1 month ago

Sure thing, @hiyoneda. And actually, following up on my initial comment, after re-reading the discussion in PR #178 I am reminded that @israelmcmc already made a good recommendation that our application have a verbosity option, as well as a flag for saving logs to file.

israelmcmc commented 1 month ago

Thanks @ckarwin. Nothing to add, really, just to paraphrase and summarize: