Ensure PeakMap log scaling applies only to colors, keeping marginal plots raw issue 65 #68
Conversation
…ues for plotting marginal distributions.
WalkthroughThe changes refactor the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plot as PeakMapPlot
User->>Plot: Instantiate (optionally with z_log_scale=True)
Note over Plot: Stores original intensity if z_log_scale is True
User->>Plot: Call prepare_data()
Plot->>Plot: Convert intensity to relative values & bin peaks\nApply log transformation on z if required
User->>Plot: Call plot()
Plot->>Plot: Ensure data is prepared\nCheck for 'add_marginals'
alt Marginals Required
Plot->>Plot: Call create_main_plot_marginals()
end
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
|
@maintainers I haven’t completed 100% testing yet, so please hold off on reviewing it for now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_core.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
40-40: Undefined name BaseMSPlot
(F821)
138-138: Undefined name plot_marginal_histogram
(F821)
🪛 GitHub Actions: continuous-integration
pyopenms_viz/_core.py
[error] 40-40: NameError: name 'BaseMSPlot' is not defined
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pyopenms_viz/_config.py (2)
451-452: Remove duplicate code setting z_log_scale for marginal plots.The settings for ensuring marginals use raw intensity values appear twice - once at lines 451-452 and again at lines 483-484. This duplication should be removed to avoid potential maintenance issues.
# Keep lines 451-452 self.y_plot_config.z_log_scale = False self.x_plot_config.z_log_scale = False # Remove lines 483-484 -# Ensure marginals always use raw intensity values -self.y_plot_config.z_log_scale = False -self.x_plot_config.z_log_scale = FalseAlso applies to: 483-484
474-479: Remove duplicate code for z-setting and annotation data.This is another instance of duplicate code between your new implementation and the existing code. The same logic for handling when
fill_by_zis False and copying annotation data appears in both places.# Keep lines 474-479 # Remove lines 515-521 -# If we do not want to fill/color based on z value, set to none prior to plotting -if not self.fill_by_z: - self.z = None - -# create copy of annotation data -self.annotation_data = ( - None if self.annotation_data is None else self.annotation_data.copy() -)Also applies to: 515-521
pyopenms_viz/_core.py (2)
39-40: Remove unused imports.The imports for
IS_SPHINX_BUILD,IS_NOTEBOOK, andwarningsare not used in this file.-from .constants import IS_SPHINX_BUILD, IS_NOTEBOOK -import warnings +# Remove unused imports🧰 Tools
🪛 Ruff (0.8.2)
39-39:
.constants.IS_SPHINX_BUILDimported but unusedRemove unused import
(F401)
39-39:
.constants.IS_NOTEBOOKimported but unusedRemove unused import
(F401)
40-40:
warningsimported but unusedRemove unused import:
warnings(F401)
149-149: Add type annotation for 'figure'.The return type
"figure"is used but not defined or imported, which could cause confusion or type checking issues.-def create_x_axis_plot(self, canvas=None) -> "figure": +def create_x_axis_plot(self, canvas=None) -> Any: # Or import the specific figure typeAlternatively, you could import the appropriate figure type from your plotting backend (matplotlib, bokeh, etc.).
🧰 Tools
🪛 Ruff (0.8.2)
149-149: Undefined name
figure(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyopenms_viz/_config.py(2 hunks)pyopenms_viz/_core.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
39-39: .constants.IS_SPHINX_BUILD imported but unused
Remove unused import
(F401)
39-39: .constants.IS_NOTEBOOK imported but unused
Remove unused import
(F401)
40-40: warnings imported but unused
Remove unused import: warnings
(F401)
149-149: Undefined name figure
(F821)
🔇 Additional comments (5)
pyopenms_viz/_config.py (1)
415-417: Good addition of log scaling control attributes.The new attributes
z_log_scale_colorsandz_log_scale_intensityprovide a clear way to control log scaling behavior, ensuring colors are log-scaled but raw intensities are preserved.pyopenms_viz/_core.py (4)
5-5: Good job addressing import issues.Nice work fixing the import issues mentioned in previous reviews for
BaseMSPlotandplot_marginal_histogram.Also applies to: 36-36
53-70: Well-implemented constructor with intensity preservation logic.The constructor properly combines the previously separate
__init__methods and correctly handles log scaling. Good defensive programming by storingself.z_originalregardless of whether log scaling is applied.
94-113: Good data preparation implementation.The
prepare_datamethod correctly handles data preparation including validation, relative intensity conversion, log scaling, and data sorting. The validation check forself.zis a good defensive programming practice.
78-90:❓ Verification inconclusive
Verify that plot_marginals() is called appropriately.
The
plot_marginals()method is well-implemented to ensure raw intensity values are used in marginal plots. However, it's unclear if this method is being properly called from theplot()method or elsewhere.Run this script to verify if
plot_marginals()is called within the codebase:
🏁 Script executed:
#!/bin/bash # Search for calls to plot_marginals in the codebase rg -A 1 -B 1 "plot_marginals\(\)"Length of output: 236
ACTION: Verify Integration of
plot_marginals()in the Plotting WorkflowOur investigation indicates that while
plot_marginals()is correctly implemented to ensure raw intensity values are used in the marginal plots, a direct call to this method does not appear in the codebase. The search results only show calls to methods likecreate_main_plot_marginals()andcreate_x_axis_plot(), with no explicit invocation ofplot_marginals().
- Confirm whether
plot_marginals()is intended to be called directly (or indirectly within another method such ascreate_main_plot_marginals()).- If the marginal plotting functionality relies on an explicit call to
plot_marginals(), please integrate that call within the main plotting workflow.- Otherwise, if this method is now redundant or its functionality is fully encapsulated by other methods, consider removing or refactoring it to avoid confusion.
| self.y_plot_config.xlabel = self.zlabel | ||
| self.y_plot_config.ylabel = self.ylabel | ||
| self.x_plot_config.ylabel = self.zlabel | ||
| self.y_plot_config.y_axis_location = "left" | ||
| self.x_plot_config.y_axis_location = "right" | ||
|
|
||
| # ✅ Update default settings for better visualization | ||
| self.y_plot_config.legend_config.show = True | ||
| self.y_plot_config.legend_config.loc = "below" | ||
| self.y_plot_config.legend_config.orientation = "horizontal" | ||
| self.y_plot_config.legend_config.bbox_to_anchor = (1, -0.4) | ||
|
|
||
| # ✅ Remove titles from marginal plots | ||
| if self.add_marginals: | ||
| self.title = "" | ||
| self.x_plot_config.title = "" | ||
| self.y_plot_config.title = "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate duplicate configuration code.
There's significant duplication between the new section (lines 455-471) and the existing code below (lines 487-512). Both blocks update the same configuration settings. This duplication creates maintenance challenges and potential inconsistencies.
Remove the redundant code by keeping only one set of these configurations:
- Label updating
- Legend configuration
- Title removal
- Positioning settings
# Keep lines 455-471 with all the settings
# Remove redundant lines 487-512
-# update y-axis labels and positioning to defaults
-self.y_plot_config.xlabel = self.zlabel
-self.y_plot_config.ylabel = self.ylabel
-self.y_plot_config.y_axis_location = "left"
-self.y_plot_config.legend_config.show = True
-self.y_plot_config.legend_config.loc = "below"
-self.y_plot_config.legend_config.orientation = "horizontal"
-self.y_plot_config.legend_config.bbox_to_anchor = (1, -0.4)
-self.y_plot_config.min_border = 0
-self.y_plot_config.height = self.height
-if self.y_kind == "spectrum":
- self.y_plot_config.direction = "horizontal"
-# update x-axis labels and positioning to defaults
-self.x_plot_config.ylabel = self.zlabel
-self.x_plot_config.y_axis_location = "right"
-self.x_plot_config.legend_config.show = True
-self.x_plot_config.legend_config.loc = "right"
-self.x_plot_config.width = self.width
-self.x_plot_config.min_border = 0
-# remove titles if marginal plot
-if self.add_marginals:
- self.title = ""
- self.x_plot_config.title = ""
- self.y_plot_config.title = ""Make sure to add back any critical settings from the removed section that aren't in the new code, like:
# Add these lines after line 458
+self.y_plot_config.min_border = 0
+self.y_plot_config.height = self.height
+if self.y_kind == "spectrum":
+ self.y_plot_config.direction = "horizontal"
+self.x_plot_config.width = self.width
+self.x_plot_config.min_border = 0Also applies to: 487-512
|
Have not done a complete review yet however, it seems that we are getting some failed tests. I expect some to fail since you are updating the code however I am concerned with the documentation not building. Can you please look into this? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app.py(1 hunks)pyopenms_viz/_bokeh/core.py(1 hunks)pyopenms_viz/_core.py(2 hunks)pyopenms_viz/_matplotlib/core.py(5 hunks)test/peaktest.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app.py
🧰 Additional context used
🪛 Ruff (0.8.2)
pyopenms_viz/_bokeh/core.py
563-563: bokeh.plotting.figure imported but unused
Remove unused import: bokeh.plotting.figure
(F401)
564-564: bokeh.models.ColorBar imported but unused
Remove unused import
(F401)
564-564: bokeh.models.LinearColorMapper imported but unused
Remove unused import
(F401)
567-567: Local variable log_intensity is assigned to but never used
Remove assignment to unused variable log_intensity
(F841)
567-567: Undefined name np
(F821)
🔇 Additional comments (11)
test/peaktest.py (2)
6-30: Well structured parametrized tests for PeakMap functionality.The test suite effectively validates different configurations of the
plotmethod, particularly testing the log scaling feature. It's good to see explicit assertions confirming that log-scaled intensities have no negative values.
33-41: Good test for marginal plots using raw intensities.This test properly verifies that marginal plots use raw intensity values even when log scaling is applied to the main plot. The assertion for
z_originalattribute ensures the implementation behaves as expected.pyopenms_viz/_bokeh/core.py (3)
594-602: Raw intensity values correctly used in X-axis marginal histogram.The implementation properly checks for the presence of
z_originalcolumn and uses it for the histogram, ensuring that raw intensity values are displayed.
607-622: Raw intensity values correctly used in Y-axis marginal histogram.Similar to the X-axis plot, this implementation properly checks for the presence of
z_originalcolumn and uses it for the histogram. This ensures consistency across both marginal plots.
588-602:⚠️ Potential issueMissing numpy import.
The code uses
np.log1p()but thenumpymodule is referred to asnpwithout an import statement.+import numpy as npLikely an incorrect or invalid review comment.
pyopenms_viz/_core.py (3)
1121-1128: Good implementation of z_log_scale parameter and original intensity preservation.The constructor correctly initializes the
z_log_scaleparameter and preserves the original intensity values inz_originalbefore any transformations are applied. This is essential for maintaining the raw data for marginal plots.
1134-1147: Well-documented data preparation method.The
prepare_datamethod now properly saves the original intensity values and applies log scaling only to thecolor_intensitycolumn when requested. The docstring clearly explains the purpose of the method.
1183-1204: Clear implementation of marginal plots with raw intensities.The
create_main_plot_marginalsmethod ensures that marginal plots use raw intensities fromz_originaleven when log scaling is applied to the main plot. The implementation is well-documented and configures the subplot space appropriately.pyopenms_viz/_matplotlib/core.py (3)
632-654: Well-documented X-axis marginal plot with raw intensity values.The
create_x_axis_plotmethod properly checks for and usesz_originaldata for the histogram, ensuring raw intensity values are displayed. Good docstring and explanatory comments.
657-698: Y-axis marginal plot correctly uses raw intensity data.The implementation properly selects
z_originalwhen available for the marginal plots. Clear documentation and comments explain the purpose of the changes.
699-734: Main plot correctly applies log scaling only to colors.The
create_main_plotmethod now correctly applies log scaling only to the color mapping usinglog_intensity, while preserving raw data for other purposes. The implementation is well-documented with clear comments.
| def create_main_plot(self, canvas=None): | ||
| """ | ||
| Implements PeakMap plotting for Bokeh. | ||
| - Applies log scaling only to color mapping. | ||
| - Uses raw intensities for marginal histograms. | ||
| """ | ||
| from bokeh.plotting import figure | ||
| from bokeh.models import ColorBar, LinearColorMapper | ||
|
|
||
| if not self.plot_3d: | ||
| # ✅ Apply log scaling **only for PeakMap color scale** | ||
| log_intensity = np.log1p(self.data[self.z]) if self.z_log_scale else self.data[self.z] | ||
|
|
||
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | ||
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | ||
|
|
||
| tooltips, custom_hover_data = self._create_tooltips( | ||
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | ||
| ) | ||
| tooltips, custom_hover_data = self._create_tooltips( | ||
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | ||
| ) | ||
|
|
||
| fig = scatterPlot.generate(tooltips, custom_hover_data) | ||
| self.main_fig = fig # Save the main figure for later use | ||
| # ✅ Use log-transformed intensity only for color mapping | ||
| fig = scatterPlot.generate(tooltips, custom_hover_data) | ||
| self.main_fig = fig # Save the main figure for later use | ||
|
|
||
| if self.annotation_data is not None: | ||
| self._add_box_boundaries(self.annotation_data) | ||
| if self.annotation_data is not None: | ||
| self._add_box_boundaries(self.annotation_data) | ||
|
|
||
| else: | ||
| raise NotImplementedError("3D PeakMap plots are not supported in Bokeh") | ||
| return fig |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log scaling implementation correctly applied only to color mapping.
The create_main_plot function now correctly applies log scaling selectively. However, there are some issues with unused imports and variables that should be addressed.
-from bokeh.plotting import figure
-from bokeh.models import ColorBar, LinearColorMapper
+# Remove unused imports
-# ✅ Apply log scaling **only for PeakMap color scale**
-log_intensity = np.log1p(self.data[self.z]) if self.z_log_scale else self.data[self.z]
+# ✅ Apply log scaling **only for PeakMap color scale**
+# Use the transformed data directly where needed instead of creating an unused variableThe log_intensity variable is calculated but never used. The transformation should be applied directly where needed or the variable should be used in the subsequent plotting code.
📝 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.
| def create_main_plot(self, canvas=None): | |
| """ | |
| Implements PeakMap plotting for Bokeh. | |
| - Applies log scaling only to color mapping. | |
| - Uses raw intensities for marginal histograms. | |
| """ | |
| from bokeh.plotting import figure | |
| from bokeh.models import ColorBar, LinearColorMapper | |
| if not self.plot_3d: | |
| # ✅ Apply log scaling **only for PeakMap color scale** | |
| log_intensity = np.log1p(self.data[self.z]) if self.z_log_scale else self.data[self.z] | |
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | |
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | |
| tooltips, custom_hover_data = self._create_tooltips( | |
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | |
| ) | |
| tooltips, custom_hover_data = self._create_tooltips( | |
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | |
| ) | |
| fig = scatterPlot.generate(tooltips, custom_hover_data) | |
| self.main_fig = fig # Save the main figure for later use | |
| # ✅ Use log-transformed intensity only for color mapping | |
| fig = scatterPlot.generate(tooltips, custom_hover_data) | |
| self.main_fig = fig # Save the main figure for later use | |
| if self.annotation_data is not None: | |
| self._add_box_boundaries(self.annotation_data) | |
| if self.annotation_data is not None: | |
| self._add_box_boundaries(self.annotation_data) | |
| else: | |
| raise NotImplementedError("3D PeakMap plots are not supported in Bokeh") | |
| return fig | |
| def create_main_plot(self, canvas=None): | |
| """ | |
| Implements PeakMap plotting for Bokeh. | |
| - Applies log scaling only to color mapping. | |
| - Uses raw intensities for marginal histograms. | |
| """ | |
| # Removed unused imports | |
| # ✅ Apply log scaling **only for PeakMap color scale** | |
| # Use the transformed data directly where needed instead of creating an unused variable | |
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | |
| tooltips, custom_hover_data = self._create_tooltips( | |
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | |
| ) | |
| # ✅ Use log-transformed intensity only for color mapping | |
| fig = scatterPlot.generate(tooltips, custom_hover_data) | |
| self.main_fig = fig # Save the main figure for later use | |
| if self.annotation_data is not None: | |
| self._add_box_boundaries(self.annotation_data) | |
| return fig |
🧰 Tools
🪛 Ruff (0.8.2)
563-563: bokeh.plotting.figure imported but unused
Remove unused import: bokeh.plotting.figure
(F401)
564-564: bokeh.models.ColorBar imported but unused
Remove unused import
(F401)
564-564: bokeh.models.LinearColorMapper imported but unused
Remove unused import
(F401)
567-567: Local variable log_intensity is assigned to but never used
Remove assignment to unused variable log_intensity
(F841)
567-567: Undefined name np
(F821)
|
Hey @t0mdavid-m @jcharkow , I have made the following improvements to the PeakMap plotting logic: ✅ Applied log scaling only to color mapping** ( Please review and let me know if any modifications are needed. Thanks! |
Summary of Changes & Fixes🔹 Log Scaling Applied Only to Colors Previously, log scaling affected the entire dataset, distorting histograms. X and Y histograms should display raw intensity values, not log-transformed ones. |
|
@Imama-Kainat can you please link to the issue this is solving (e.g. edit the description and add the issue that this addresses) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pyopenms_viz/_core.py (3)
1121-1122: Remove redundant pandas import.pandas is already imported at line 10 with specific components, making this import unnecessary.
- import pandas as pd
1123-1135: Good implementation for preserving original intensity values.You've correctly added the z_log_scale parameter and implemented proper logic to store original intensity values before transformation. The error handling for various edge cases is thorough.
However, note the inconsistency with how original values are stored: here you set
self.z_original, but inprepare_data()you usedata["z_original"].Consider using a consistent approach to store original values - either as an instance attribute or as a dataframe column, not both.
🧰 Tools
🪛 Ruff (0.8.2)
1129-1129: Undefined name
pd(F821)
1191-1212: Excellent fix for marginal plots using raw intensities.This implementation ensures that marginal histograms display raw intensity values instead of log-transformed ones, which addresses the core issue described in the PR objectives.
The method properly:
- Calls the abstract create_main_plot() function
- Adds histograms that use z_original for intensity values
- Keeps subplot spacing and layout consistent
Some matplotlib-specific code is embedded here, which might be better in the matplotlib implementation.
Consider whether this matplotlib-specific implementation should be moved to the matplotlib backend class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_core.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
1129-1129: Undefined name pd
(F821)
🔇 Additional comments (1)
pyopenms_viz/_core.py (1)
1141-1155: Well-structured data preparation logic.The
prepare_datamethod clearly separates the concerns by:
- Preserving original intensity values
- Only applying log scaling to color mapping
- Using descriptive variable names for clarity
This implementation correctly addresses the issue where log scaling was previously applied to both colors and marginal plots.
Imama-Kainat
changed the title
|
|
|
singjc
left a comment
singjc
left a comment
There was a problem hiding this comment.
Thanks for the PR, this is a good first attempt, but there are a few issues.
- There are some additional variables you create, which don't actually seem to get used anywhere. i.e (
log_intensity,self.data["color_intensity"]) - You add additional histogram marginal plots, not sure why?
- I'm not sure what the purpose of the added circle glyphs for bokeh are for?
- We probably don't want to remove the option of plotting relative intensity or perform binning on the data.
- We would want to also ensure the same logic gets added/applied to the plotly backend.
Overall, there just seems to be a lot of code changes, where I'm not sure what the purpose of the changes are, and why we're removing some parts that seem necessary.
I was originally thinking all we would need to change is the z value data used in pyopenms_viz/_core.py#L1229 and pyopenms_viz/_core.py#L1264. This would only require changes to the _core.py file, and wouldn't require changes to the individual backends. I would maybe suggest starting a fresh PR with this implementation idea (since this one seem so to have a lot of changes).
| def create_main_plot(self, canvas=None): | ||
| """ | ||
| Implements PeakMap plotting for Bokeh. | ||
| - Applies log scaling only to color mapping. | ||
| - Uses raw intensities for marginal histograms. | ||
| """ | ||
| from bokeh.plotting import figure | ||
| from bokeh.models import ColorBar, LinearColorMapper | ||
|
|
||
| if not self.plot_3d: | ||
| # ✅ Apply log scaling **only for PeakMap color scale** | ||
| log_intensity = np.log1p(self.data[self.z]) if self.z_log_scale else self.data[self.z] | ||
|
|
||
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | ||
| scatterPlot = self.get_scatter_renderer(data=self.data, config=self._config) | ||
|
|
||
| tooltips, custom_hover_data = self._create_tooltips( | ||
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | ||
| ) | ||
| tooltips, custom_hover_data = self._create_tooltips( | ||
| {self.xlabel: self.x, self.ylabel: self.y, "intensity": self.z} | ||
| ) | ||
|
|
||
| fig = scatterPlot.generate(tooltips, custom_hover_data) | ||
| self.main_fig = fig # Save the main figure for later use | ||
| # ✅ Use log-transformed intensity only for color mapping | ||
| fig = scatterPlot.generate(tooltips, custom_hover_data) | ||
| self.main_fig = fig # Save the main figure for later use | ||
|
|
||
| if self.annotation_data is not None: | ||
| self._add_box_boundaries(self.annotation_data) | ||
| if self.annotation_data is not None: | ||
| self._add_box_boundaries(self.annotation_data) | ||
|
|
||
| else: | ||
| raise NotImplementedError("3D PeakMap plots are not supported in Bokeh") | ||
| return fig |
There was a problem hiding this comment.
Please fix the indentation of this method.
|
|
||
| return fig | ||
|
|
||
|
|
| - Applies log scaling only to color mapping. | ||
| - Uses raw intensities for marginal histograms. |
There was a problem hiding this comment.
Can you put these under a note
| - Applies log scaling only to color mapping. | |
| - Uses raw intensities for marginal histograms. | |
| Notes | |
| ----- | |
| Applies log scaling only for the color scale of the PeakMap, and uses the raw values (i.e. intensities) for marginal plots (i.e. spectrum, chromatogram, mobilogram). |
| from bokeh.models import ColorBar, LinearColorMapper | ||
|
|
||
| if not self.plot_3d: | ||
| # ✅ Apply log scaling **only for PeakMap color scale** |
There was a problem hiding this comment.
Can remove this comment
| # ✅ Apply log scaling **only for PeakMap color scale** |
|
|
||
| fig = scatterPlot.generate(tooltips, custom_hover_data) | ||
| self.main_fig = fig # Save the main figure for later use | ||
| # ✅ Use log-transformed intensity only for color mapping |
There was a problem hiding this comment.
Can remove this comment
| # ✅ Use log-transformed intensity only for color mapping |
| # ✅ Apply log scaling **only for PeakMap colors**. | ||
| if self.z_log_scale: | ||
| self.data[self.z] = log1p(self.data[self.z]) | ||
| self.data["color_intensity"] = np.log1p(self.data[self.z]) |
There was a problem hiding this comment.
Does self.data["color_intensity"] even get used anywhere in the backends plotting logic?
| # Sort values by intensity in ascending order to plot highest intensity peaks last | ||
| if self.z is not None: | ||
| self.data = self.data.sort_values(self.z) |
There was a problem hiding this comment.
If we don't sort the intensities, then you will get plots where the high intensities will get masked/covered by other intensities, which doesn't result in a nice visualization.
| def create_main_plot_marginals(self, canvas=None): | ||
| """ | ||
| Calls the abstract create_main_plot() function but ensures: | ||
| - Log scaling is only applied to PeakMap colors. | ||
| - Marginal plots use raw intensities (`z_original`). | ||
| """ | ||
| fig = self.create_main_plot(canvas) | ||
|
|
||
| if self.add_marginals: | ||
| fig.subplots_adjust(hspace=0.2, wspace=0.2) | ||
| grid = fig.add_gridspec(4, 4) | ||
|
|
||
| ax_marg_x = fig.add_subplot(grid[0, 0:3], sharex=fig.axes[0]) | ||
| ax_marg_x.hist(self.data["z_original"], bins=30, color="gray", alpha=0.6) | ||
| ax_marg_x.axis("off") | ||
|
|
||
| ax_marg_y = fig.add_subplot(grid[1:4, 3], sharey=fig.axes[0]) | ||
| ax_marg_y.hist(self.data["z_original"], bins=30, orientation="horizontal", color="gray", alpha=0.6) | ||
| ax_marg_y.axis("off") | ||
|
|
||
| return fig |
There was a problem hiding this comment.
This needs to be indented, but also why is there added code here for adding the marginals? This is handled by the main plot method? pyopenms_viz/_core.py#L1159-L1169
The marginals also should not be histograms, they should be of kind chromatogram, spectrum or mobilogram.
|
|
||
| # ✅ Ensure raw intensity values are used for the marginal histogram | ||
| if "z_original" in self.data.columns: | ||
| ax.hist(self.data["z_original"], bins=30, color="gray", alpha=0.6) |
There was a problem hiding this comment.
Why are we adding a histogram here to the x-axis marginal plot?
| x=self.x, | ||
| y=self.y, | ||
| z=self.z, | ||
| z=log_intensity, # ✅ Only log-transform color scale |
There was a problem hiding this comment.
I think z is expected to be a string for the column variable, not a numpy array.
4 participants
🔹 What This PR Fixes
This PR fixes the issue where log scaling was incorrectly applied to both PeakMap colors and marginal plots. Now:
Log scaling is applied only to self.z for PeakMap visualization. ✅
Marginal plots always display raw intensity values (self.z_original). ✅
Avoids unintended log scaling side effects on other properties. ✅
🔹 Changes Made
1️⃣ Stored original intensity values before applying log transformation (self.z_original).
2️⃣ Updated plot_marginals() to always use self.z_original (raw intensities).
3️⃣ Ensured log scaling applies only to self.z (color mapping), not marginal plots.
Summary by CodeRabbit
New Features
Bug Fixes