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

Draft: POC #607

Closed
wants to merge 8 commits into from
Closed

Draft: POC #607

wants to merge 8 commits into from

Conversation

vschaffn
Copy link

@vschaffn vschaffn commented Oct 21, 2024

Replace print statements with logging

Resolves #565

Summary

This pull request addresses the transition from using print()statements to the Python logging module. The update improves flexibility for managing output messages.

Changes made:

  • Replaces all occurences of print with appropriate logging levels (debug, info, warning, error)
  • Removed the verbose flag in favor of log levels
  • Replaced all f-strings with % formatting in logging statements to comply with pylint requirements

Documentation

Updated the documentation to include instructions on how users can configure logging verbosity.

@adebardo adebardo self-requested a review October 22, 2024 06:36
Copy link

@adebardo adebardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work, it's great.

  • Could you also provide a detailed description in your PR to summarize your work so that other reviewers can quickly get the information about your PR?

  • The idea would be to separate your test commits from the code modifications more clearly in the future.

  • I still think that modifying an example to demonstrate the use of logging could be relevant.

@@ -74,7 +74,7 @@ def test_init(self) -> None:
if "NaNs found in dDEM" not in str(exception):
raise exception

# print(cumulative_dh)
# logging.info(cumulative_dh)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhugonnet, @adehecq : How should we handle this line that appears to be debugging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can leave as is or simply delete!
The DEMCollection is an old module from the start of the package, that we are discussing about removing or re-factoring from scratch, so it's not very important.

@@ -189,12 +188,10 @@ def _iterate_method(

# Print final results
# TODO: Allow to pass a string to _iterate_method on how to print/describe exactly the iterating input
if verbose:
pbar.write(f" Iteration #{i + 1:d} - Offset: {new_inputs}; Magnitude: {new_statistic}")
logging.debug(" Iteration #%d - Offset: %s; Magnitude: %s", i + 1, new_inputs, new_statistic)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pbar.write really just a simple print?

name: xdem
---
# Logging configuration
TO configure logging for xDEM, you can utilize Python's built-in `logging` module. Begin by setting up the logging

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first "TO" has an extra capital letter, doesn't it?

])
```
This configuration will log messages with a severity level of `INFO` and above, including timestamps, logger names, and
log levels in the output. You can change the logging level to `DEBUG`, `WARNING`, `ERROR` or `CRITICAL` as needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you manage to build the documentation?

@adebardo
Copy link

NB: Do not merge this branch; it serves as a base for a POC branch.

@adebardo adebardo changed the title POC Draft: POC Oct 22, 2024
# Conflicts:
#	doc/source/logging.md
#	tests/test_coreg/test_affine.py
#	tests/test_coreg/test_base.py
#	tests/test_coreg/test_biascorr.py
#	tests/test_spatialstats.py
#	xdem/coreg/affine.py
#	xdem/coreg/base.py
#	xdem/coreg/workflows.py
#	xdem/spatialstats.py
#	xdem/volume.py
@adebardo
Copy link

Copy of MR #610

@adebardo adebardo closed this Oct 22, 2024
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 this pull request may close these issues.

[POC] use logging module
3 participants