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

Replace print statements and verbose flag with logging and log levels #610

Merged
merged 4 commits into from
Oct 28, 2024

Conversation

vschaffn
Copy link
Contributor

@vschaffn vschaffn commented Oct 22, 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:

  • Replaced 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

Note on tdqm and logging integration in xdem.correg.affine.py

Since tdqm directly manages output to the console, mixing its output with standard logging (especially at lower verbosity levels) can lead to overlapping or garbled output.

To addess this, I ensured that:

  • The pbar(from tdqm.trange) is conditionnally displayed depending on the current logging level. When the logging level is set to INFO or higher, the progress bar is visible.
  • pbar.write() is used for logging messages within the loop, after checking the logging level.

Documentation

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

Example

Added an example showing how a users can use and configure logging

@adebardo adebardo mentioned this pull request Oct 22, 2024
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.

@adehecq @rhugonnet @duboise-cnes We reviewed it together with @vschaffn, what do you think of this first contribution?

@duboise-cnes
Copy link
Member

@adehecq @rhugonnet @duboise-cnes We reviewed it together with @vschaffn, what do you think of this first contribution?

Seems ok from my view of the xdem code. Maybe not useful to put the example from now and maybe not at this place ?
It would be put into a pipeline code but don't know if it is necessary. @adehecq @rhugonnet your opinon ?
Otherwise, i let Glaciohack say if it is ok for xdem.

As said before, the goal is to merge CNES/CS contributions into this branch-poc branch to aggregate contributions to xdem. @adehecq @rhugonnet feel free to propose other contribution way

@rhugonnet
Copy link
Contributor

@vschaffn @adebardo This is great, thanks! 😄

Good initative for the example, and documentation page! I think your new documentation page (that is not listed in the index.md right now) would fit very well in the "Reference" section of the doc (with API/Background currently, and where there would be CLI/Config section mirroring the doc of GeoUtils: https://geoutils.readthedocs.io/en/stable/), if this organization makes sense to you all as well?

For the example (not sure I'm commenting on the same commits), I agree with @duboise-cnes that it might not be the best spot, or maybe too generic. I could see it live well in the current "Basic" examples if re-structured into a Sphinx-Gallery format as the others, and calling a typical xDEM function (instead of divide) to show the logging levels (like NuthKaab?).
There is a bit of redundancy between documentation pages and Sphinx-Gallery examples, but this is intended as different users like to learn in different ways.

In terms of the code modifications: It all looks great! I would add a note in the PR description about how you dealt with the interface between tqdm and logging, which doesn't seem trivial, for posterity.

Happy with the organization of merging to GlacioHack:branch-poc. Given the nature of the POC changes, I think there should be minimal conflicts with other changes ongoing on the main branch this fall. But if you'd prefer merging each distinct contribution (like the "logging" one, which is fully documented) individually to main to avoid accumulating too much on branch-poc and potentially get more merge conflicts, this would work as well. 😉

@adebardo
Copy link

Thank you for your feedback, @duboise-cnes and @rhugonnet .

  • Indeed, this example seems problematic; we will propose another solution.
  • We will also add a note in the PR regarding tqdm.
  • Perhaps we could do a mix of both: we can continue working on branch-poc, and if it becomes too complicated, we will do more frequent merges into main.

@vschaffn
Copy link
Contributor Author

@rhugonnet @duboise-cnes I have taken your feedbacks into account:

  • I have changed the interactions between tqdm and logging in xdem.coreg.affine.py to avoid any conflict between the two in the console and added a note about this in the PR.
  • I have modified the logging example to be more xdem-specific (using NuthKaab coregistration) and restructured it to match the Sphinx-Gallery format.

@vschaffn vschaffn changed the base branch from branch-poc to main October 28, 2024 09:20
@vschaffn vschaffn changed the title 565- [POC] use logging module Replace print statements and verbose flag with logging and log levels Oct 28, 2024
@adebardo adebardo merged commit 6384b5d into GlacioHack:main Oct 28, 2024
@vschaffn vschaffn deleted the poc branch November 26, 2024 09:49
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
4 participants