feat: update plot correlation functions #104
Conversation
|
Warning Rate limit exceeded@strixy16 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 1 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 (2)
WalkthroughThe pull request introduces several new functionalities to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/readii/analyze/plot_correlation.py (2)
191-192: Optional docstring improvement.The return statement is straightforward, but consider adding descriptive text to the docstring near lines 191-192 to provide clarity about the returned values, especially if future maintainers need more context on bin_values versus bin_edges.
225-262: Save directory & error handling.
- The usage of PlotWriter to control output directories under lines 235-255 is a good approach for consistent file saving.
- The conditional check at lines 248-249 logs a warning if a file already exists, preventing accidental overwrites. This is excellent for data integrity.
- Consider adding a try-except around the actual saving (line 254) if you want to log or handle file I/O errors more gracefully, although the current approach might suffice if PlotWriter handles exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/readii/analyze/plot_correlation.py(3 hunks)
🔇 Additional comments (2)
src/readii/analyze/plot_correlation.py (2)
Line range hint 1-12: Imports and integration.
These newly added imports for getCrossCorrelations, getSelfCorrelations, and PlotWriter are appropriate and clearly integrate with the existing correlation and plotting code. Ensure that the respective modules are available at runtime and that you maintain consistent versioning of these dependencies within the project.
193-224: Function naming and usage clarity.
The function name plotSelfCorrHeatmap accurately describes its purpose. The introductory docstring is also consistent with the code. The parameter naming (especially feature_type_name, correlation_method, cmap, save_dir_path) is self-explanatory, enhancing readability.
| vertical_feature_name:str, | ||
| horizontal_feature_name:str, | ||
| correlation_method:str = "pearson", | ||
| cmap:str='nipy_spectral', | ||
| save_dir_path:Optional[str] = None) -> tuple[Figure | Figure, Path]: | ||
| """Plot a heatmap of the cross correlations from a correlation matrix. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| correlation_matrix : pd.DataFrame | ||
| Dataframe containing the correlation matrix to plot. | ||
| vertical_feature_name : str | ||
| Name of the vertical feature to get cross correlations for. Must be the suffix of some feature names in the correlation matrix index. | ||
| horizontal_feature_name : str | ||
| Name of the horizontal feature to get cross correlations for. Must be the suffix of some feature names in the correlation matrix columns. | ||
| correlation_method : str, optional | ||
| Method to use for calculating correlations. Default is "pearson". | ||
| cmap : str, optional | ||
| Colormap to use for the heatmap. Default is "nipy_spectral". | ||
| save_path : str, optional | ||
| Path to save the heatmap to. If None, the heatmap will not be saved. Default is None. | ||
| File will be saved to {save_dir_path}/heatmap/{cmap}/{vertical_feature_name}_vs_{horizontal_feature_name}_{correlation_method}_cross_correlation_heatmap.png | ||
|
|
||
| Returns | ||
| ------- | ||
| cross_corr_heatmap : matplotlib.pyplot.figure | ||
| Figure object containing the heatmap of the cross correlations. | ||
| if save_path is not None: | ||
| cross_corr_save_path : Path | ||
| Path to the saved heatmap. | ||
| """ | ||
| # Get the cross correlations for the specified feature type | ||
| cross_corr = getCrossCorrelations(correlation_matrix, vertical_feature_name, horizontal_feature_name) | ||
|
|
||
| # Make the heatmap figure | ||
| cross_corr_heatmap = plotCorrelationHeatmap(cross_corr, | ||
| diagonal=True, | ||
| cmap=cmap, | ||
| xlabel=vertical_feature_name, | ||
| ylabel=horizontal_feature_name, | ||
| title=f"{correlation_method.capitalize()} Cross Correlations", subtitle=f"{vertical_feature_name} vs {horizontal_feature_name}") | ||
|
|
||
| if save_dir_path is not None: | ||
| # Create a PlotWriter instance to save the heatmap | ||
| heatmap_writer = PlotWriter(root_directory = save_dir_path / "heatmap", | ||
| filename_format = "{ColorMap}/" + "{VerticalFeatureType}_vs_{HorizontalFeatureType}_{CorrelationType}_cross_correlation_heatmap.png", | ||
| overwrite = False, | ||
| create_dirs = True | ||
| ) | ||
|
|
||
| # Get the output path for the heatmap | ||
| cross_corr_save_path = heatmap_writer.resolve_path(VerticalFeatureType = vertical_feature_name, | ||
| HorizontalFeatureType = horizontal_feature_name, | ||
| CorrelationType = correlation_method, | ||
| ColorMap = cmap) | ||
|
|
||
| # Check if the heatmap already exists | ||
| if cross_corr_save_path.exists(): | ||
| logger.warning(f"Correlation heatmap already exists at {cross_corr_save_path}.") | ||
|
|
||
| else: | ||
| logger.debug("Saving correlation heatmap.") | ||
| # Save the heatmap | ||
| cross_corr_save_path = heatmap_writer.save(cross_corr_heatmap, | ||
| VerticalFeatureType = vertical_feature_name, | ||
| HorizontalFeatureType = horizontal_feature_name, | ||
| CorrelationType = correlation_method, | ||
| ColorMap = cmap) | ||
| # Return the figure and the path to the saved heatmap | ||
| return cross_corr_heatmap, cross_corr_save_path | ||
|
|
||
| else: | ||
| # Return the heatmap figure | ||
| return cross_corr_heatmap |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Parameter name mismatch and diagonal setting are valid concerns
- The docstring incorrectly references "save_path" while the function uses "save_dir_path".
- Setting diagonal=True in plotCorrelationHeatmap for cross-correlations is incorrect. The getCrossCorrelations function returns a matrix where rows and columns represent different feature types (vertical vs. horizontal), so there's no meaningful diagonal to mask. The diagonal=True parameter would unnecessarily hide part of the cross-correlation data.
- Update the docstring to use "save_dir_path" instead of "save_path"
- Change
diagonal=Truetodiagonal=Falsein the plotCorrelationHeatmap call to show the complete cross-correlation matrix
🔗 Analysis chain
Docstring parameter mismatch and diagonal setting for cross correlation.
- In lines 285-287, the docstring references a parameter named “save_path,” whereas the function parameter is “save_dir_path.” Rename either the docstring parameter or the function parameter to maintain consistency.
- The heatmap calls plotCorrelationHeatmap with diagonal=True for cross correlation. While this may be intentional to highlight only half the matrix, consider verifying whether a full matrix display (diagonal=False) is more appropriate for cross correlations.
- Overall logic is consistent: the function retrieves cross correlations, plots them, and optionally saves the figure with a well-structured naming convention.
Let me gather more information about the cross-correlation plotting functionality to verify both concerns.
Let me check the implementation of getCrossCorrelations to understand if diagonal=True is appropriate for cross-correlations.
Let me try a different pattern to get the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check references to save_path or save_dir_path in docstrings vs. function signatures
rg -A 3 "def plotCrossCorrHeatmap"
Length of output: 417
Script:
#!/bin/bash
# Check the implementation of plotCorrelationHeatmap to understand diagonal parameter usage
ast-grep --pattern 'def plotCorrelationHeatmap($$$)'
# Search for other cross-correlation related functions to understand the context
rg -B 2 -A 5 "getCrossCorrelations"
Length of output: 3559
Script:
#!/bin/bash
# Check the implementation of getCrossCorrelations to understand the matrix structure
ast-grep --pattern 'def getCrossCorrelations($$$) {
$$$
}'
Length of output: 62
Script:
#!/bin/bash
# Check the implementation of getCrossCorrelations
rg -B 2 -A 20 "^def getCrossCorrelations"
Length of output: 2165
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 41.91% 47.66% +5.75%
==========================================
Files 33 33
Lines 1453 1502 +49
==========================================
+ Hits 609 716 +107
+ Misses 844 786 -58 ☔ View full report in Codecov by Sentry. |
This makes checking for a file's existence prior to saving easier
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/analyze/test_plot_correlation.py (1)
83-84: Remove f-string prefixThe string doesn't contain any placeholders, so the f-string prefix is unnecessary.
- assert corr_fig.get_suptitle() == f"Correlation Heatmap", \ + assert corr_fig.get_suptitle() == "Correlation Heatmap", \🧰 Tools
🪛 Ruff (0.8.2)
83-83: f-string without any placeholders
Remove extraneous
fprefix(F541)
src/readii/analyze/plot_correlation.py (1)
259-572: Consider performance optimization for large matricesFor large correlation matrices, consider:
- Using numpy's optimized functions for matrix operations
- Implementing caching for frequently accessed correlation data
- Adding progress logging for long-running operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/readii/analyze/plot_correlation.py(5 hunks)src/readii/io/writers/plot_writer.py(2 hunks)tests/analyze/test_plot_correlation.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/analyze/test_plot_correlation.py
2-2: readii.io.writers.plot_writer.PlotWriter imported but unused
Remove unused import: readii.io.writers.plot_writer.PlotWriter
(F401)
12-12: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
83-83: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (6)
src/readii/io/writers/plot_writer.py (1)
22-25: LGTM: Well-structured exception handling enhancement
The addition of PlotWriterPlotExistsError improves error handling specificity for existing plot scenarios.
tests/analyze/test_plot_correlation.py (1)
18-132: LGTM: Comprehensive test coverage
The test suite provides thorough coverage of the correlation plotting functionality:
- Well-structured fixtures for test data
- Parameterized tests for different correlation types
- Proper validation of output paths and file existence
🧰 Tools
🪛 Ruff (0.8.2)
83-83: f-string without any placeholders
Remove extraneous f prefix
(F541)
src/readii/analyze/plot_correlation.py (4)
17-49: LGTM: Well-implemented save functionality
The saveCorrelationHeatmap function is well-structured with proper:
- Error handling using the new PlotWriterPlotExistsError
- Path construction for different feature types
- Logging of warnings and debug information
287-287: Fix docstring parameter name mismatch
The docstring references save_path but the parameter is named save_dir_path.
- if save_path is not None:
+ if save_dir_path is not None:Also applies to: 361-361
295-296: Verify diagonal parameter usage for self-correlation
Setting diagonal=True for self-correlation heatmaps is correct as it helps visualize the symmetry of self-correlations.
369-370: LGTM: Correct diagonal parameter for cross-correlation
Setting diagonal=False for cross-correlation heatmaps is correct as cross-correlations don't have meaningful diagonal elements.
| from readii.analyze.correlation import getFeatureCorrelations | ||
| from readii.io.writers.plot_writer import PlotWriter |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused imports
The imports PlotWriter and Path are not used in the test file.
-from readii.io.writers.plot_writer import PlotWriter
-from pathlib import PathAlso applies to: 12-12
🧰 Tools
🪛 Ruff (0.8.2)
2-2: readii.io.writers.plot_writer.PlotWriter imported but unused
Remove unused import: readii.io.writers.plot_writer.PlotWriter
(F401)
| cross_corr_hist = plotCorrelationHistogram(cross_corr, | ||
| num_bins=num_bins, | ||
| xlabel = f"{correlation_method.capitalize()} Correlation", | ||
| y_lower_bound = y_lower_bound, | ||
| y_upper_bound = y_upper_bound, | ||
| title = f"Distribution of {correlation_method.capitalize()} Cross Correlations", | ||
| subtitle=f"{vertical_feature_name} vs {horizontal_feature_name}") |
There was a problem hiding this comment.
Fix inconsistent return value unpacking
The function call to plotCorrelationHistogram returns a tuple of three values, but only one is being assigned.
- cross_corr_hist = plotCorrelationHistogram(cross_corr,
+ cross_corr_hist, _, _ = plotCorrelationHistogram(cross_corr, 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Make the histogram figure | |
| cross_corr_hist = plotCorrelationHistogram(cross_corr, | |
| num_bins=num_bins, | |
| xlabel = f"{correlation_method.capitalize()} Correlation", | |
| y_lower_bound = y_lower_bound, | |
| y_upper_bound = y_upper_bound, | |
| title = f"Distribution of {correlation_method.capitalize()} Cross Correlations", | |
| subtitle=f"{vertical_feature_name} vs {horizontal_feature_name}") | |
| # Make the histogram figure | |
| cross_corr_hist, _, _ = plotCorrelationHistogram(cross_corr, | |
| num_bins=num_bins, | |
| xlabel = f"{correlation_method.capitalize()} Correlation", | |
| y_lower_bound = y_lower_bound, | |
| y_upper_bound = y_upper_bound, | |
| title = f"Distribution of {correlation_method.capitalize()} Cross Correlations", | |
| subtitle=f"{vertical_feature_name} vs {horizontal_feature_name}") |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/analyze/test_plot_correlation.py (1)
68-71: Avoid hardcoding test values.The test uses hardcoded values (4.0 and 30.0) for bin validation. Consider:
- Using dynamic calculation based on the input data
- Adding tolerance for floating-point comparisons
- Documenting why these specific values are expected
- assert bin_values[0] == 4.0, \ - f"Wrong first bin value, expect 4.0, got {bin_values[0]}" - assert bin_values[-1] == 30.0, \ - f"Wrong last value, expect 30.0, got {bin_values[-1]}" + # Calculate expected values based on the correlation matrix + expected_first_bin = len([x for x in correlation_matrix.values.flatten() if -1.0 <= x < -0.98]) + expected_last_bin = len([x for x in correlation_matrix.values.flatten() if 0.98 <= x <= 1.0]) + + np.testing.assert_allclose(bin_values[0], expected_first_bin, rtol=1e-10, + err_msg=f"Wrong first bin value, expect {expected_first_bin}, got {bin_values[0]}") + np.testing.assert_allclose(bin_values[-1], expected_last_bin, rtol=1e-10, + err_msg=f"Wrong last bin value, expect {expected_last_bin}, got {bin_values[-1]}")src/readii/analyze/plot_correlation.py (1)
487-493: Consider adding correlation range to histogram title.For better context, consider including the correlation range in the histogram title or subtitle:
cross_corr_hist, _, _ = plotCorrelationHistogram(cross_corr, num_bins=num_bins, xlabel = f"{correlation_method.capitalize()} Correlation", y_lower_bound = y_lower_bound, y_upper_bound = y_upper_bound, - title = f"Distribution of {correlation_method.capitalize()} Cross Correlations", + title = f"Distribution of {correlation_method.capitalize()} Cross Correlations (Range: -1 to 1)", subtitle=f"{vertical_feature_name} vs {horizontal_feature_name}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/readii/analyze/plot_correlation.py(5 hunks)tests/analyze/test_plot_correlation.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/analyze/test_plot_correlation.py
2-2: readii.io.writers.plot_writer.PlotWriter imported but unused
Remove unused import: readii.io.writers.plot_writer.PlotWriter
(F401)
16-16: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
87-87: f-string without any placeholders
Remove extraneous f prefix
(F541)
146-146: f-string without any placeholders
Remove extraneous f prefix
(F541)
159-159: f-string without any placeholders
Remove extraneous f prefix
(F541)
171-171: f-string without any placeholders
Remove extraneous f prefix
(F541)
184-184: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (5)
tests/analyze/test_plot_correlation.py (2)
1-2: Remove unused imports.
The imports PlotWriter and Path are not used in the test file.
🧰 Tools
🪛 Ruff (0.8.2)
2-2: readii.io.writers.plot_writer.PlotWriter imported but unused
Remove unused import: readii.io.writers.plot_writer.PlotWriter
(F401)
22-52: LGTM! Well-structured test fixtures.
The fixtures provide a clean setup for the test environment with:
- Temporary directory for correlations
- Feature names for vertical and horizontal axes
- Correlation method specification
- Random correlation matrix generation
src/readii/analyze/plot_correlation.py (3)
347-347:
Fix docstring parameter name mismatch.
The docstring references 'save_path' but the parameter is named 'save_dir_path':
- if save_path is not None:
+ if save_dir_path is not None:Likely invalid or redundant comment.
287-287:
Fix docstring parameter name mismatch.
The docstring references 'save_path' but the parameter is named 'save_dir_path':
- if save_path is not None:
+ if save_dir_path is not None:Likely invalid or redundant comment.
355-357:
Remove diagonal masking for cross-correlation heatmap.
Setting diagonal=True for cross-correlations is incorrect as the matrix represents correlations between different feature types. The diagonal has no special meaning in this context.
cross_corr_heatmap = plotCorrelationHeatmap(cross_corr,
- diagonal=False,
cmap=cmap, Likely invalid or redundant comment.
| "Wrong title, expect Correlation Heatmap" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "correlation_type, feature_types", | ||
| [ | ||
| ("pearson_self", ["vertical"]), | ||
| ("pearson_cross", ["vertical", "horizontal"]), | ||
| ] | ||
| ) | ||
| def test_save_corr_heatmap(correlation_matrix, correlations_dir, correlation_type, feature_types): | ||
| """Test saving a heatmap from a cross-correlation matrix""" | ||
| corr_fig = plotCorrelationHeatmap(correlation_matrix) | ||
|
|
||
| expected_path = correlations_dir / "heatmap" / "nipy_spectral" / ("_vs_".join(feature_types)) + f"_{correlation_type}_correlation_heatmap.png" | ||
|
|
||
| actual_path = saveCorrelationHeatmap(corr_fig, | ||
| correlations_dir, | ||
| cmap="nipy_spectral", | ||
| feature_types=feature_types, | ||
| correlation_type=correlation_type) | ||
| assert actual_path == expected_path, \ | ||
| "Wrong path returned, expect {expected_path}" | ||
| assert actual_path.exists(), \ | ||
| "Figure is not being saved to path provided or at all." | ||
|
|
||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "correlation_type, feature_types", | ||
| [ | ||
| ("pearson_self", ["vertical"]), | ||
| ("pearson_cross", ["vertical", "horizontal"]), | ||
| ] | ||
| ) | ||
| def test_save_corr_histogram(correlation_matrix, correlations_dir, correlation_type, feature_types): | ||
| """Test saving a histogram from a correlation matrix""" | ||
| corr_fig, _, _ = plotCorrelationHistogram(correlation_matrix) | ||
|
|
||
| expected_path = correlations_dir / "histogram" / ("_vs_".join(feature_types)) + f"_{correlation_type}_correlation_histogram.png" | ||
|
|
||
| actual_path = saveCorrelationHistogram(corr_fig, | ||
| correlations_dir, | ||
| feature_types=feature_types, | ||
| correlation_type=correlation_type) | ||
| assert actual_path == expected_path, \ | ||
| "Wrong path returned, expect {expected_path}" | ||
| assert actual_path.exists(), \ | ||
| "Figure is not being saved to path provided or at all." | ||
|
|
||
|
|
||
|
|
||
| def test_plot_selfcorr_heatmap_defaults(correlation_matrix, vertical_feature_name): | ||
| """Test plotting a self-correlation heatmap from a correlation matrix""" | ||
| corr_fig = plotSelfCorrHeatmap(correlation_matrix, | ||
| feature_type_name=vertical_feature_name) | ||
| assert isinstance(corr_fig, Figure), \ | ||
| "Wrong return type, expect a matplotlib Figure" | ||
| assert corr_fig.get_suptitle() == f"Pearson Self Correlations", \ | ||
| "Wrong title, expect Pearson Self Correlations" | ||
| assert corr_fig.get_axes()[0].get_title(), \ | ||
| "Wrong subtitle, expect vertical" | ||
|
|
||
|
|
||
| def test_plot_crosscorr_heatmap_defaults(correlation_matrix, vertical_feature_name, horizontal_feature_name): | ||
| """Test plotting a cross-correlation heatmap from a correlation matrix""" | ||
| corr_fig = plotCrossCorrHeatmap(correlation_matrix, | ||
| vertical_feature_name=vertical_feature_name, | ||
| horizontal_feature_name=horizontal_feature_name) | ||
| assert isinstance(corr_fig, Figure), \ | ||
| "Wrong return type, expect a matplotlib Figure" | ||
| assert corr_fig.get_suptitle() == f"Pearson Cross Correlations", \ | ||
| "Wrong title, expect Pearson Cross Correlations" | ||
| assert corr_fig.get_axes()[0].get_title(), \ | ||
| "Wrong subtitle, expect vertical vs horizontal" | ||
|
|
||
|
|
||
| def test_plot_selfcorr_histogram_defaults(correlation_matrix, vertical_feature_name): | ||
| """Test plotting a self-correlation histogram from a correlation matrix""" | ||
| corr_fig = plotSelfCorrHistogram(correlation_matrix, | ||
| feature_type_name=vertical_feature_name) | ||
| assert isinstance(corr_fig, Figure), \ | ||
| "Wrong return type, expect a matplotlib Figure" | ||
| assert corr_fig.get_suptitle() == f"Distribution of Pearson Self Correlations", \ | ||
| "Wrong title, expect Distribution of Pearson Self Correlations" | ||
| assert corr_fig.get_axes()[0].get_title(), \ | ||
| "Wrong subtitle, expect vertical" | ||
|
|
||
|
|
||
| def test_plot_crosscorr_histogram_defaults(correlation_matrix, vertical_feature_name, horizontal_feature_name): | ||
| """Test plotting a cross-correlation histogram from a correlation matrix""" | ||
| corr_fig = plotCrossCorrHistogram(correlation_matrix, | ||
| vertical_feature_name=vertical_feature_name, | ||
| horizontal_feature_name=horizontal_feature_name) | ||
| assert isinstance(corr_fig, Figure), \ | ||
| "Wrong return type, expect a matplotlib Figure" | ||
| assert corr_fig.get_suptitle() == f"Distribution of Pearson Cross Correlations", \ | ||
| "Wrong title, expect Pearon Cross Correlations" | ||
| assert corr_fig.get_axes()[0].get_title(), \ | ||
| "Wrong subtitle, expect vertical vs horizontal" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error case tests.
The test suite lacks coverage for error scenarios. Consider adding tests for:
- Invalid correlation methods
- Non-existent feature names
- Invalid bin counts
- File permission errors when saving
Example test case:
def test_invalid_bin_count():
"""Test that negative bin count raises ValueError."""
with pytest.raises(ValueError, match="Number of bins must be positive"):
plotCorrelationHistogram(correlation_matrix, num_bins=-1)🧰 Tools
🪛 Ruff (0.8.2)
87-87: f-string without any placeholders
Remove extraneous f prefix
(F541)
146-146: f-string without any placeholders
Remove extraneous f prefix
(F541)
159-159: f-string without any placeholders
Remove extraneous f prefix
(F541)
171-171: f-string without any placeholders
Remove extraneous f prefix
(F541)
184-184: f-string without any placeholders
Remove extraneous f prefix
(F541)
| assert actual_path == expected_path, \ | ||
| "Wrong path returned, expect {expected_path}" |
There was a problem hiding this comment.
Fix f-string formatting in assertion message.
The f-string is missing the f prefix:
- assert actual_path == expected_path, \
- "Wrong path returned, expect {expected_path}"
+ assert actual_path == expected_path, \
+ f"Wrong path returned, expect {expected_path}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert actual_path == expected_path, \ | |
| "Wrong path returned, expect {expected_path}" | |
| assert actual_path == expected_path, \ | |
| f"Wrong path returned, expect {expected_path}" |
| assert corr_fig.get_suptitle() == f"Distribution of Pearson Cross Correlations", \ | ||
| "Wrong title, expect Pearon Cross Correlations" |
There was a problem hiding this comment.
Fix typo in assertion message.
There's a typo in the expected title:
- assert corr_fig.get_suptitle() == f"Distribution of Pearson Cross Correlations", \
- "Wrong title, expect Pearon Cross Correlations"
+ assert corr_fig.get_suptitle() == "Distribution of Pearson Cross Correlations", \
+ "Wrong title, expect Distribution of Pearson Cross Correlations"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert corr_fig.get_suptitle() == f"Distribution of Pearson Cross Correlations", \ | |
| "Wrong title, expect Pearon Cross Correlations" | |
| assert corr_fig.get_suptitle() == "Distribution of Pearson Cross Correlations", \ | |
| "Wrong title, expect Distribution of Pearson Cross Correlations" |
🧰 Tools
🪛 Ruff (0.8.2)
184-184: f-string without any placeholders
Remove extraneous f prefix
(F541)
| assert actual_path == expected_path, \ | ||
| "Wrong path returned, expect {expected_path}" |
There was a problem hiding this comment.
Fix f-string formatting in assertion message.
The f-string is missing the f prefix:
- assert actual_path == expected_path, \
- "Wrong path returned, expect {expected_path}"
+ assert actual_path == expected_path, \
+ f"Wrong path returned, expect {expected_path}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert actual_path == expected_path, \ | |
| "Wrong path returned, expect {expected_path}" | |
| assert actual_path == expected_path, \ | |
| f"Wrong path returned, expect {expected_path}" |
| correlation_directory:Path, | ||
| feature_types:list[str], | ||
| correlation_type:str, | ||
| overwrite:bool = False) -> None: |
There was a problem hiding this comment.
Fix incorrect return type hint.
The function returns a Path but the type hint indicates None:
- overwrite:bool = False) -> None:
+ overwrite:bool = False) -> Path:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| overwrite:bool = False) -> None: | |
| overwrite:bool = False) -> Path: |
add self and cross correlation plot functions
Summary by CodeRabbit