Skip to content

fix: replace debug print() statements with logger.debug() in metric modules - #81

Closed
Balamuralikrishna123 wants to merge 4 commits into
idtlab:developfrom
Balamuralikrishna123:fix/replace-print-with-logging
Closed

fix: replace debug print() statements with logger.debug() in metric modules#81
Balamuralikrishna123 wants to merge 4 commits into
idtlab:developfrom
Balamuralikrishna123:fix/replace-print-with-logging

Conversation

@Balamuralikrishna123

Copy link
Copy Markdown

hi, i was going through the codebase and noticed issue #71
about replacing print() statements with proper logging.
i went through all the metric modules and replaced the debug
print() calls with logger.debug() and added the logger
setup at the top of each file where it was missing.
files changed:

  • feature_relevance.py
  • class_imbalance.py
  • conditional_demo_disp.py
  • correlation_score.py
  • privacy_measure.py
    this fixes the issue and makes the debug output properly
    controllable through python logging levels instead of
    always printing to stdout.
    also working on the zarr reader in PR feat: add Zarr file format support #76 and planning
    to tackle the ROOT reader from issue Add root file reader #49 next.

@Balamuralikrishna123

Copy link
Copy Markdown
Author

the lint failure appears to be related to the deprecated Node.js 20 GitHub Actions ) rather than the code changes. the same warning appears across other PRs in the repo as well

@Aman-Cool

Copy link
Copy Markdown
Contributor

Hey @Balamuralikrishna123, thanks for the contribution, but the PR needs a cleanup before we can merge.

The zarr reader changes (zarr_reader.py, file_parser.py) don't belong here, that's PR #76. Not sure how they ended up in this branch, but please rebase and drop those commits. Also, the branch currently has merge conflicts that need to be resolved, resolving them after dropping those commits should clean things up.

One thing I'd encourage, make sure you're reading through the changes before submitting. It's easy to miss things like bundled unrelated code, and we want contributors who are comfortable owning their changes. So just a quick sanity check:

  • Why logger.debug() and not logger.info()?
  • Why define the logger at module level?

Answer those, clean up the branch, and we'll get this merged!

@Aman-Cool

Copy link
Copy Markdown
Contributor

@kaveenh, please have a look!

@Aman-Cool Aman-Cool mentioned this pull request Mar 18, 2026
9 tasks
@Balamuralikrishna123

Balamuralikrishna123 commented Mar 18, 2026

Copy link
Copy Markdown
Author

Hey @Balamuralikrishna123, thanks for the contribution, but the PR needs a cleanup before we can merge.

The zarr reader changes (zarr_reader.py, file_parser.py) don't belong here, that's PR #76. Not sure how they ended up in this branch, but please rebase and drop those commits. Also, the branch currently has merge conflicts that need to be resolved, resolving them after dropping those commits should clean things up.

One thing I'd encourage, make sure you're reading through the changes before submitting. It's easy to miss things like bundled unrelated code, and we want contributors who are comfortable owning their changes. So just a quick sanity check:

  • Why logger.debug() and not logger.info()?
  • Why define the logger at module level?

Answer those, clean up the branch, and we'll get this merged!

Hey @Balamuralikrishna123, thanks for the contribution, but the PR needs a cleanup before we can merge.

The zarr reader changes (zarr_reader.py, file_parser.py) don't belong here, that's PR #76. Not sure how they ended up in this branch, but please rebase and drop those commits. Also, the branch currently has merge conflicts that need to be resolved, resolving them after dropping those commits should clean things up.

One thing I'd encourage, make sure you're reading through the changes before submitting. It's easy to miss things like bundled unrelated code, and we want contributors who are comfortable owning their changes. So just a quick sanity check:

  • Why logger.debug() and not logger.info()?
  • Why define the logger at module level?

Answer those, clean up the branch, and we'll get this merged!

hi @Aman-Cool , thanks for pointing that out , yeah i noticed the zarr files got bundled in by mistake, i will clean that up and push a fix shortly.
for your questions:

  1. i went with logger.debug() because these print statements looked like they were there for debugging purposes, not for regular info output. debug level only shows up when someone explicitly sets logging to DEBUG so it wont clutter normal output.
  2. module level logger is the standard way i have seen it done in most python projects. using name gives each module its own logger which is cleaner than defining it inside every function separately.will clean up the branch now and push. Thank you.

@Aman-Cool

Copy link
Copy Markdown
Contributor

@Balamuralikrishna123, I noticed this bundling in #82 too...have a look.

@Balamuralikrishna123

Balamuralikrishna123 commented Mar 18, 2026

Copy link
Copy Markdown
Author

opened a clean version without the zarr changes in PR #83, please review that one instead. sorry for the mess.

@kaveenh

kaveenh commented Mar 18, 2026

Copy link
Copy Markdown
Collaborator

@Balamuralikrishna123 Thanks for your contributions! Please avoid creating multiple PRs for the same changes. Please update or close existing PRs so maintainers can manage the repository efficiently.

@kaveenh kaveenh closed this Mar 18, 2026
@Balamuralikrishna123

Copy link
Copy Markdown
Author

@Balamuralikrishna123 Thanks for your contributions! Please avoid creating multiple PRs for the same changes. Please update or close existing PRs so maintainers can manage the repository efficiently.

hey @kaveenh understood, sorry for the confusion! PR #83 is the clean version with only the logging changes.

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.

3 participants