Enhance x-tick label customization: add font size, color, and rotatio… #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n options; improve handling of x-tick labels across subplots
Reviewer's GuideThis PR enriches the plot_peptigram function in viz.py with extensive label and legend customization (font sizes, colors, rotations, wrapping, sample bars and subtitles), refactors its internal styling logic, updates documentation, fixes CSV column handling in DataProcessor, adjusts the default runner pipeline to use sample color bars, and tweaks package initialization including a version bump. Sequence diagram for sample color bar legend creation in plot_peptigramsequenceDiagram
participant User
participant plot_peptigram
participant Matplotlib
User->>plot_peptigram: Call with use_sample_color_bars=True, sample_colors, etc.
plot_peptigram->>Matplotlib: Add vertical colored bars for each sample
plot_peptigram->>Matplotlib: Create sample legend below protein legend
plot_peptigram->>Matplotlib: Add notes for coloring method and regions
Matplotlib-->>User: Display plot with sample color bars and enhanced legend
Class diagram for updated plot_peptigram function parametersclassDiagram
class plot_peptigram {
+color_by: str
+figsize: Tuple[int, int]
+title: Optional[str]
+y_desnity_forntsize: int
+y_desnity_forntcolour: str
+y_lab_ticksize: int
+y_sample_fontsize: int
+y_sample_color: Optional[Union[list, str]]
+max_sample_name_length: int
+sample_name_wrap: bool
+use_sample_color_bars: bool
+sample_colors: Optional[List[str]]
+sample_bar_width: float
+x_lab_forntsize: int
+xticks_font: int
+xticks_color: str
+xticks_rotation: int
+annotate: bool
+legend_titleFontsize: int
+legend_fontsize: int
+min_intensity: Optional[float]
+highlight_regions: Optional[List[Tuple[int, int]]]
+auto_highlight: bool
+auto_highlight_threshold: float
+highlight: bool
+color_by_protein_and_intensity: bool
+colour_by_text: bool
+intensity_color_scale: float
+intensity_cmaps: Union[str, List[str]]
+protein_cmap: str
...
}
Class diagram for DataProcessor CSV handling changesclassDiagram
class DataProcessor {
+peaks_data: pd.DataFrame
+intensity_cols: List[str]
+load_peaks_data(file_path: str) : pd.DataFrame
+filter_and_format_data(...)
}
DataProcessor : load_peaks_data() renames columns to capitalized
DataProcessor : filter_and_format_data() strips sample_prefix and leading symbols from sample names
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sanjaysgk - I've reviewed your changes - here's some feedback:
- The plot_peptigram signature has grown to 30+ parameters—consider grouping related style options into a config object or named dict to reduce signature bloat and improve usability.
- There are multiple typos in parameter names (e.g. “y_desnity_forntsize”, “forntcolour”)—renaming them to correct spellings will avoid confusion and ensure consistency.
- The large inline logic for sample color bars, label wrapping, and legend positioning could be extracted into smaller helper functions to improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The plot_peptigram signature has grown to 30+ parameters—consider grouping related style options into a config object or named dict to reduce signature bloat and improve usability.
- There are multiple typos in parameter names (e.g. “y_desnity_forntsize”, “forntcolour”)—renaming them to correct spellings will avoid confusion and ensure consistency.
- The large inline logic for sample color bars, label wrapping, and legend positioning could be extracted into smaller helper functions to improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `ProtPeptigram/viz.py:51` </location>
<code_context>
color_by: str = None,
figsize: Tuple[int, int] = (12, 10),
title: Optional[str] = None,
+ y_desnity_forntsize: int = 8,
+ y_desnity_forntcolour: str = "#333333",
+ y_lab_ticksize: int = 8,
+ y_sample_fontsize: int = 8,
+ y_sample_color: Optional[Union[list, str]] = "#333333",
+ max_sample_name_length: int = 15,
+ sample_name_wrap: bool = True,
</code_context>
<issue_to_address>
Several new parameter names contain typos ('forntsize', 'desnity', etc.).
Please correct the spelling of parameter names to avoid confusion and maintain consistency.
Suggested implementation:
```python
y_density_fontsize: int = 8,
y_density_fontcolor: str = "#333333",
```
```python
x_lab_fontsize: int = 12,
```
You will also need to update all usages of these parameters within the function body and any calls to this function elsewhere in the codebase to use the corrected names:
- `y_density_fontsize` instead of `y_desnity_forntsize`
- `y_density_fontcolor` instead of `y_desnity_forntcolour`
- `x_lab_fontsize` instead of `x_lab_forntsize`
</issue_to_address>
### Comment 2
<location> `ProtPeptigram/viz.py:56` </location>
<code_context>
+ y_lab_ticksize: int = 8,
+ y_sample_fontsize: int = 8,
+ y_sample_color: Optional[Union[list, str]] = "#333333",
+ max_sample_name_length: int = 15,
+ sample_name_wrap: bool = True,
+ use_sample_color_bars: bool = False,
+ sample_colors: Optional[List[str]] = None,
+ sample_bar_width: float = 0.5,
x_lab_forntsize: int = 12,
- y_lab_forntsize: int = 12,
</code_context>
<issue_to_address>
Default value for max_sample_name_length in docstring does not match function signature.
Please update the docstring to reflect the correct default value for max_sample_name_length.
</issue_to_address>
### Comment 3
<location> `ProtPeptigram/viz.py:60` </location>
<code_context>
+ sample_name_wrap: bool = True,
+ use_sample_color_bars: bool = False,
+ sample_colors: Optional[List[str]] = None,
+ sample_bar_width: float = 0.5,
x_lab_forntsize: int = 12,
- y_lab_forntsize: int = 12,
</code_context>
<issue_to_address>
sample_bar_width is described as a float but used as a linewidth in axvline, which expects points.
Clarify in the docstring that sample_bar_width is in points, and consider increasing the default value for better visibility.
Suggested implementation:
```python
sample_bar_width: float = 2.0,
```
You must also update the docstring for the function or class where these parameters are defined. Add or modify the description for `sample_bar_width` to clarify that it is in points (as used by `axvline`). For example:
sample_bar_width (float): Width of the sample color bars in points (as used by matplotlib's axvline). Default is 2.0.
Make sure this clarification is present in the docstring.
</issue_to_address>
### Comment 4
<location> `ProtPeptigram/viz.py:603` </location>
<code_context>
+
+ # Add colored vertical bar for each sample if requested
+ if use_sample_color_bars:
+ if sample_colors is None:
+ # Generate colors automatically
+ sample_cmap = plt.cm.get_cmap('tab10')
+ sample_color = sample_cmap(i % sample_cmap.N)
+ else:
+ sample_color = sample_colors[i % len(sample_colors)]
+
+ # Add vertical colored bar on the left
</code_context>
<issue_to_address>
sample_legend_handles is initialized inside the loop, which may cause issues.
If the loop is skipped or i != 0 initially, sample_legend_handles may be undefined, causing a NameError. Initialize it before the loop to avoid this issue.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+55
| y_desnity_forntsize: int = 8, | ||
| y_desnity_forntcolour: str = "#333333", | ||
| y_lab_ticksize: int = 8, | ||
| y_sample_fontsize: int = 8, | ||
| y_sample_color: Optional[Union[list, str]] = "#333333", |
There was a problem hiding this comment.
suggestion (typo): Several new parameter names contain typos ('forntsize', 'desnity', etc.).
Please correct the spelling of parameter names to avoid confusion and maintain consistency.
Suggested implementation:
y_density_fontsize: int = 8,
y_density_fontcolor: str = "#333333", x_lab_fontsize: int = 12,You will also need to update all usages of these parameters within the function body and any calls to this function elsewhere in the codebase to use the corrected names:
y_density_fontsizeinstead ofy_desnity_forntsizey_density_fontcolorinstead ofy_desnity_forntcolourx_lab_fontsizeinstead ofx_lab_forntsize
Comment on lines
+56
to
+60
| max_sample_name_length: int = 15, | ||
| sample_name_wrap: bool = True, | ||
| use_sample_color_bars: bool = False, | ||
| sample_colors: Optional[List[str]] = None, | ||
| sample_bar_width: float = 0.5, |
There was a problem hiding this comment.
issue: Default value for max_sample_name_length in docstring does not match function signature.
Please update the docstring to reflect the correct default value for max_sample_name_length.
| sample_name_wrap: bool = True, | ||
| use_sample_color_bars: bool = False, | ||
| sample_colors: Optional[List[str]] = None, | ||
| sample_bar_width: float = 0.5, |
There was a problem hiding this comment.
suggestion: sample_bar_width is described as a float but used as a linewidth in axvline, which expects points.
Clarify in the docstring that sample_bar_width is in points, and consider increasing the default value for better visibility.
Suggested implementation:
sample_bar_width: float = 2.0,You must also update the docstring for the function or class where these parameters are defined. Add or modify the description for sample_bar_width to clarify that it is in points (as used by axvline). For example:
sample_bar_width (float): Width of the sample color bars in points (as used by matplotlib's axvline). Default is 2.0.
Make sure this clarification is present in the docstring.
Comment on lines
+603
to
+608
| if sample_colors is None: | ||
| # Generate colors automatically | ||
| sample_cmap = plt.cm.get_cmap('tab10') | ||
| sample_color = sample_cmap(i % sample_cmap.N) | ||
| else: | ||
| sample_color = sample_colors[i % len(sample_colors)] |
There was a problem hiding this comment.
issue (bug_risk): sample_legend_handles is initialized inside the loop, which may cause issues.
If the loop is skipped or i != 0 initially, sample_legend_handles may be undefined, causing a NameError. Initialize it before the loop to avoid this issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…n options; improve handling of x-tick labels across subplots
Summary by Sourcery
Enhance the plot_peptigram function with extensive axis label and legend customization, sample labeling options including wrapping and colored bars, streamline sample name extraction, and update package versioning and entrypoint handling
New Features:
Bug Fixes:
Enhancements:
Chores: