Clean code, remove print -> logger, add all authors. #100
Conversation
|
Warning Rate limit exceeded@ypriverol has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request implements widespread enhancements in logging, documentation, and type annotations across multiple modules. Logger instances with NullHandler replace direct logging calls, and several docstrings have been reformatted to a structured style. In some modules, type hints have been improved (e.g., using forward references) and error handling adjusted (e.g., raising ValueError). The CLI functionality now accepts configurable logging options, and the package metadata is updated with additional authors. Overall, the core logic remains unchanged, with modifications focused on clarity, maintainability, and logging robustness. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as ibaqpyc.cli
participant Log as Logging.Config
participant FS as FileSystem
User->>CLI: call cli(log_level, log_file)
CLI->>Log: Configure basic logging (set format, level)
alt log_file provided
CLI->>FS: Check if log_file exists
FS-->>CLI: Existence result
CLI->>Log: Create FileHandler for log_file (if needed)
end
CLI->>User: Execute CLI command with enhanced logging
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied ☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
ibaqpy/ibaq/ibaqpy_postprocessing.py (1)
264-264: Replace direct logging call with logger instance.For consistency with other changes, use the logger instance instead of directly calling
logging.warning.Apply this diff to fix the inconsistency:
- logging.warning(f"Found {long_df[values].isna().sum()} missing values in the result") + logger.warning(f"Found {long_df[values].isna().sum()} missing values in the result")ibaqpy/commands/correct_batches.py (1)
57-59: Replace print statements with logger instance.For consistency with other logging changes, use the logger instance instead of print statements.
Apply this diff to fix the inconsistency:
- print("The following sample IDs are invalid:") - for invalid_sample in invalid_samples: - print(f" - {invalid_sample}") + logger.warning("The following sample IDs are invalid:") + for invalid_sample in invalid_samples: + logger.warning(f" - {invalid_sample}")
🧹 Nitpick comments (3)
ibaqpy/ibaqpyc.py (2)
14-19: Consider adding more logging levels for comprehensive error handling.The logging configuration could be enhanced by:
- Adding "error" and "critical" levels
- Using "WARNING" instead of "WARN" for consistency with Python's logging module
-LOG_LEVELS = ["debug", "info", "warn"] +LOG_LEVELS = ["debug", "info", "warning", "error", "critical"] LOG_LEVELS_TO_LEVELS = { "debug": logging.DEBUG, "info": logging.INFO, - "warn": logging.WARN, + "warning": logging.WARNING, + "error": logging.ERROR, + "critical": logging.CRITICAL, }
40-49: Simplify nested if statements for better readability.The log file setup can be simplified by combining the conditions.
- if log_file: - if not log_file.exists(): - if not log_file.parent.exists(): - log_file.parent.mkdir(parents=True, exist_ok=True) + if log_file and not log_file.exists(): + log_file.parent.mkdir(parents=True, exist_ok=True)🧰 Tools
🪛 Ruff (0.8.2)
40-41: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
ibaqpy/ibaq/utils.py (1)
405-411: Simplify nested if statements.The nested if statements can be combined using
andfor better readability.Apply this diff to simplify the code:
- if covs: - if len(df.columns) != len(covs): + if covs and len(df.columns) != len(covs):🧰 Tools
🪛 Ruff (0.8.2)
405-406: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ibaqpy/commands/correct_batches.py(8 hunks)ibaqpy/commands/features2peptides.py(1 hunks)ibaqpy/commands/peptides2protein.py(1 hunks)ibaqpy/ibaq/file_utils.py(4 hunks)ibaqpy/ibaq/ibaqpy_commons.py(6 hunks)ibaqpy/ibaq/ibaqpy_postprocessing.py(7 hunks)ibaqpy/ibaq/peptide_normalization.py(8 hunks)ibaqpy/ibaq/utils.py(15 hunks)ibaqpy/ibaqpyc.py(2 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- ibaqpy/commands/features2peptides.py
🧰 Additional context used
🪛 Ruff (0.8.2)
ibaqpy/ibaqpyc.py
40-41: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
ibaqpy/ibaq/utils.py
405-406: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (11)
ibaqpy/ibaqpyc.py (1)
27-33: LGTM! Well-structured CLI options for logging configuration.The implementation provides good defaults and clear help text for the logging options.
ibaqpy/commands/peptides2protein.py (1)
80-87: LGTM! Clear and concise docstring update.The docstring maintains essential information while improving readability.
ibaqpy/ibaq/file_utils.py (2)
4-15: LGTM! Well-implemented type checking and logging setup.Good use of TYPE_CHECKING for forward references and proper logger configuration with NullHandler.
25-25: LGTM! Good improvements to type safety and logging.The forward reference for AnnData and the informative log message enhance code quality.
Also applies to: 112-112
ibaqpy/ibaq/ibaqpy_commons.py (1)
98-98: LGTM! Improved type annotations and documentation.Good improvements:
- More accurate return types using Figure instead of matplotlib.pyplot
- Well-structured numpy-style docstrings
- Clear parameter and return type documentation
Also applies to: 132-132, 176-194, 197-224, 227-243
ibaqpy/ibaq/ibaqpy_postprocessing.py (1)
18-19: LGTM! Logger configuration follows best practices.The logger is correctly configured with a NullHandler, which prevents "No handlers could be found" warnings when the application doesn't configure logging.
ibaqpy/commands/correct_batches.py (1)
18-19: LGTM! Logger configuration follows best practices.The logger is correctly configured with a NullHandler, which prevents "No handlers could be found" warnings when the application doesn't configure logging.
ibaqpy/ibaq/utils.py (1)
16-17: LGTM! Logger configuration follows best practices.The logger is correctly configured with a NullHandler, which prevents "No handlers could be found" warnings when the application doesn't configure logging.
ibaqpy/ibaq/peptide_normalization.py (2)
34-35: LGTM! Logger configuration follows best practices.The logger is correctly configured with a NullHandler and uses a descriptive name that follows the package hierarchy.
457-457: LGTM! Error handling improvement.The error handling is improved by raising a ValueError with a clear message and preserving the original exception using the
fromclause.pyproject.toml (1)
11-13: New Authors Added Successfully
The three new author entries are correctly formatted and integrated into the authors array. This update aligns well with the metadata enhancements described in the PR objectives. Consider verifying that all new contributors are also mentioned in the project documentation for consistency.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
tests/test_file_utils.py (1)
31-31: Use module-level logger instead of root logger.Using the root logger directly with
logging.info()is not recommended. Instead, follow the pattern used in other test files by configuring a module-level logger with NullHandler.+logger = logging.getLogger(__name__) +logger.addHandler(logging.NullHandler()) - logging.info(df_ibaq.head()) + logger.info(df_ibaq.head()) - logging.info(adata) + logger.info(adata)Also applies to: 59-59
tests/test_batch_correction.py (1)
40-40: Use consistent logger throughout the test.The test uses both the root logger (
logging.debug()) and the module-level logger (logger.info()). Stick to using the module-level logger consistently.- logging.debug("Arguments for run_batch_correction: %s", args) + logger.debug("Arguments for run_batch_correction: %s", args)Also applies to: 55-55
🧹 Nitpick comments (12)
ibaqpy/ibaq/utils.py (2)
404-409: Simplify nested if statements.The nested if statements can be combined using
andfor better readability.- if covs: - if len(df.columns) != len(covs): + if covs and len(df.columns) != len(covs):🧰 Tools
🪛 Ruff (0.8.2)
404-405: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
502-502: Fix logger.info string formatting.Use format string or % formatting instead of string concatenation for better logging performance.
- logger.info("Running iteration: ", i + 1) + logger.info("Running iteration: %d", i + 1)ibaqpy/ibaq/peptides2protein.py (4)
43-44: LGTM! Consider adding type hints for the logger.The logger setup follows best practices. Consider adding type hints for better code maintainability:
-logger = logging.getLogger("ibaqpy.peptides2protein") +logger: logging.Logger = logging.getLogger("ibaqpy.peptides2protein")
128-128: Enhance error message structure.Consider using a more structured error message format that includes the count of nonstandard amino acids:
- logger.error(f"Nonstandard amino acids found in {accession}: {error_aa}, ignored!") + logger.error(f"Found {len(error_aa)} nonstandard amino acid(s) in protein {accession}: {', '.join(error_aa)}. These will be ignored.")
337-337: Enhance data preview logging.Add context and improve formatting of the DataFrame preview:
- logger.info(data.head()) + logger.info("Preview of processed peptide data:\n%s", data.head().to_string())
327-327: Address TODO comments for data integrity and documentation.There are two TODO items that should be addressed:
- Implementing duplicate checks for protein lists
- Completing the ConcentrationWeightByProteomicRuler class documentation
Would you like me to help implement the duplicate check functionality or provide a complete class documentation? I can:
- Generate code to check for and handle duplicate proteins
- Write a comprehensive docstring for the ConcentrationWeightByProteomicRuler class
Also applies to: 136-136
ibaqpy/commands/tsne_visualization.py (2)
195-195: Enhance log message with context.While converting print to logging is good, the message could be more descriptive for better debugging context.
Consider this improvement:
- logger.info(total_proteins.shape) + logger.info("Total proteins DataFrame shape: %s", total_proteins.shape)
179-187: Add error handling and progress logging for data processing steps.The DataFrame operations and dimensionality reduction steps could benefit from additional error handling and progress logging.
Consider these improvements:
normalize_df = pd.pivot_table( total_proteins, index=[SAMPLE_ID, "reanalysis"], columns=PROTEIN_NAME, values=IBAQ_LOG, ) + logger.debug("Created pivot table with shape: %s", normalize_df.shape) normalize_df = normalize_df.fillna(0) + logger.info("Starting PCA computation with %d components", 30) df_pca = compute_pca(normalize_df, n_components=30) + logger.info("Starting t-SNE computation") df_tsne = compute_tsne(df_pca) + logger.info("Completed dimensionality reduction")Also consider adding try-except blocks to handle potential DataFrame operation errors:
try: normalize_df = pd.pivot_table( total_proteins, index=[SAMPLE_ID, "reanalysis"], columns=PROTEIN_NAME, values=IBAQ_LOG, ) except KeyError as e: logger.error("Failed to create pivot table: missing required column - %s", e) raise except Exception as e: logger.error("Unexpected error during pivot table creation: %s", e) raisetests/test_ibaqpy.py (1)
43-43: Consider adding structured logging for test parameters.While the current logging is functional, consider using structured logging to make the test parameters more easily parseable:
- logger.info(args) + logger.info("Test parameters for ibaq_compute", extra={"parameters": args})tests/test_peptide_normalize.py (1)
51-51: Consider adding structured logging for test parameters.While the current logging is functional, consider using structured logging to make the test parameters more easily parseable:
- logger.info(args) + logger.info("Test parameters for feature_assembly", extra={"parameters": args})ibaqpy/ibaq/combiner.py (2)
216-216: Improve logging of filtered outliers DataFrame.Logging the entire DataFrame could produce excessive output. Consider limiting the output to key statistics or a sample of the data.
Apply this diff to improve the logging:
- logger.info(self.df_filtered_outliers) + logger.info(f"Filtered outliers DataFrame stats:\n" + f"Shape: {self.df_filtered_outliers.shape}\n" + f"Sample of data:\n{self.df_filtered_outliers.head()}")
305-305: Improve logging of corrected DataFrame.Logging the entire DataFrame could produce excessive output. Consider limiting the output to key statistics or a sample of the data.
Apply this diff to improve the logging:
- logger.info(self.df_corrected) + logger.info(f"Batch corrected DataFrame stats:\n" + f"Shape: {self.df_corrected.shape}\n" + f"Sample of data:\n{self.df_corrected.head()}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ibaqpy/commands/correct_batches.py(8 hunks)ibaqpy/commands/tsne_visualization.py(3 hunks)ibaqpy/ibaq/combiner.py(4 hunks)ibaqpy/ibaq/ibaqpy_postprocessing.py(8 hunks)ibaqpy/ibaq/peptides2protein.py(4 hunks)ibaqpy/ibaq/utils.py(17 hunks)tests/test_batch_correction.py(2 hunks)tests/test_file_utils.py(1 hunks)tests/test_ibaqpy.py(2 hunks)tests/test_peptide_normalize.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ibaqpy/ibaq/ibaqpy_postprocessing.py
- ibaqpy/commands/correct_batches.py
🧰 Additional context used
🪛 Ruff (0.8.2)
ibaqpy/ibaq/utils.py
404-405: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build (3.9)
- GitHub Check: build
🔇 Additional comments (6)
ibaqpy/ibaq/utils.py (2)
16-17: LGTM! Good logging setup.The logger configuration follows best practices by using
__name__and adding a NullHandler to prevent "No handlers could be found" warnings.
346-350: LGTM! The custom exception class is well implemented.The
TooFewSamplesInBatchclass correctly inherits fromValueErrorand properly formats the error message.ibaqpy/commands/tsne_visualization.py (1)
2-2: LGTM! Logging setup follows best practices.The logger is correctly initialized with
__name__and configured with a NullHandler, which prevents "No handlers found" warnings while allowing parent loggers to control the output.Also applies to: 15-16
tests/test_ibaqpy.py (1)
1-2: LGTM! Logger configuration follows best practices.The logger is properly configured with a NullHandler, which prevents "No handlers could be found" warnings while allowing parent loggers to handle the output if configured.
Also applies to: 9-10
tests/test_peptide_normalize.py (1)
1-2: LGTM! Logger configuration follows best practices.The logger is properly configured with a NullHandler, which prevents "No handlers could be found" warnings while allowing parent loggers to handle the output if configured.
Also applies to: 8-9
tests/test_batch_correction.py (1)
13-14: LGTM! Logger configuration follows best practices.The logger is properly configured with a NullHandler, which prevents "No handlers could be found" warnings while allowing parent loggers to handle the output if configured.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
• Enhanced the command-line interface with configurable logging options, allowing users to set custom log levels and direct output to a file for improved runtime feedback.
• Introduced logging functionality across various modules, improving error handling and output control.
Documentation
• Streamlined help texts and error messages across commands for clearer, more consistent guidance during usage.
• Updated function docstrings to a more structured format, enhancing clarity and usability.