xdem icon indicating copy to clipboard operation
xdem copied to clipboard

[POC] use logging module

Open adebardo opened this issue 1 year ago • 3 comments

Context

When using xdem errors and other types of information are displayed via print. The use of the logging module is more relevant, especially with the arrival of the CLI. This ticket, therefore proposes to replace all prints of xdem in logging.

First, it is good to read the [documentation] (https://docs.python.org/en/3/howto/logging.html) in order to master the different levels and constraints related to the use

Code

The files affected by the changes are as follows: -xdem/examples.py

  • xdem/affine.py -xdem/base.py -xdem/biascorr.py -xdem/workflows.py

  • xdem/fit.py -xdem/misc.py -xdem/spatialstats.py

  • xdem/volume.py

  • example/plot_nuth_kaab.py

  • example/plot_block_wise_coreg.py

  • example/plor_deramp.py

  • example/plot_heterosc_estimation_modelling.py

  • example/plot_standardization.py

  • example/plot_variogram_estimation_modelling.py

  • example/plot_dem_substraction.py

  • example/plot_infer_heterosc.py

  • example/plot_infer_spatial_correlation.py

  • example/plot_spatial_error_propagation.py

  • test/test_coreg_pipeline.py

  • test/test_demcollection.py

  • test/test_doc.py

  • [ ] In xdem at the moment there is a verbose variable that if enabled then prints are displayed. Therefore, this variable must be removed from the APIs.

  • [ ] replace the prints with adequate logging.debug, logging.info, etc. :warning: f-string doesn’t pass pylint with logging we recommand using the percent formatting. (%s :string, %d int, %f float, ...)

  • [ ] Add some logs in run and tests it

For CLI

  • [ ] Add a new key in config file : verbose_level : info, debug, warning, error
  • [ ] add a configuration for cli inspired by: In xdem_cli.py add the function for setup the log https://github.com/CNES/demcompare/blob/master/demcompare/log_conf.py and instanciate a file for saving https://github.com/CNES/demcompare/blob/master/demcompare/init.py#L92

Doc

  • [ ] Add a paragraph to tell the user how to enable this verbosity in CLI mode but also in API mode

Some help for an API use

import logging  
  
# Configuration 
logging.basicConfig(level=logging.DEBUG,  
                    format='%(asctime)s - %(name)s - %(levelname)s - %(message)s',  
                    datefmt='%Y-%m-%d %H:%M:%S',  
                    handlers=[  
                        logging.FileHandler('app.log'),  # saving in a file  
  logging.StreamHandler()  # Print in console  
  ])  
  
  
logger = logging.getLogger('mylogger')  
  
def division(a, b):  
    try:  
        result = a / b  
  except ZeroDivisionError:  
        logger.error("Error : division by zero")  
    else:  
        logger.info("The %s / %s = %s", a, b, result)  
        return result  
  
division(10, 2)  
division(10, 0)

Tests

  • [ ] verify that the logging file from CLI has been created

Estimation 3d

adebardo avatar Aug 22 '24 08:08 adebardo

Thanks @adebardo for the issue. It's ok for me globally. Some remarks and complements from my experience of other projects:

  • for cli configuration, we could put it in the configuration file and not keep command line parameter if easier (to be
  • I agree to find a simple way to use from CLI AND API for user to change the level of debug
  • We have also added on CARS project a PROGRESS level which is not bad for information progress in pipeline CLI. Anyway, each level have to be well defined and keep logging standards is easier for end users/developers.
  • for many reasons (performance and compatibily), f-string is not so well in logging. Python recommends old %s way. Linter can be parameter on the choice made (pylint i know, for ruff to be found...) : https://docs.python.org/3/howto/logging.html#logging-variable-data . In demcompare we were using .format method. I think it is a good to have one at the conversion and keep it all along the code for consistency (with help of the linter, this can be added to the issue).
  • compared with the example, we were using directly logging.info, logging.debug much easier to not pass the "logger" throughout the modules.
  • I think we can rely and have inspiration on demcompare parts : https://gitlab.cnes.fr/3d/demcompare/-/blob/master/demcompare/log_conf.py?ref_type=heads and https://gitlab.cnes.fr/3d/demcompare/-/blob/master/demcompare/init.py?ref_type=heads#L92

duboise-cnes avatar Aug 30 '24 08:08 duboise-cnes

Thank you for this feedback.
1 - I acknowledge the possibility of adding this to the config file indeed.
2 - I’ve noted the point about the logging format. 3 - I’ll also add more global logging management.

adebardo avatar Aug 30 '24 08:08 adebardo

It would be great to have the logging option indeed and be able to set it up through the CLI and API. We can also possibly add it to a global config file like what is done in geoutils.

  • for many reasons (performance and compatibily), f-string is not so well in logging.

Just out of curiosity, what is wrong with the f-string? Why would it have lower performance? I got used to the f-strings now because they are much easier to read. So I would try to use f-string whenever possible, i.e. outside of logging.

adehecq avatar Sep 04 '24 14:09 adehecq