Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions app.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,6 @@ def get_input_col_kind(params, plot_type):
fig = st.session_state.chrom_df.plot(**main_input_args, backend=backend_map[engine], show_plot=False, **figure_kwargs)



with tabs[0]:
display_fig(fig.fig, engine)
if common_params["extract_manual_features"]:
Expand Down Expand Up @@ -336,4 +335,4 @@ def get_input_col_kind(params, plot_type):
with tabs[1]:
st.dataframe(st.session_state.chrom_df)
with tabs[2]:
st.write(plotChromatogram)
st.write(plotChromatogram)
58 changes: 43 additions & 15 deletions pyopenms_viz/_bokeh/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -554,47 +554,75 @@ class BOKEHPeakMapPlot(BOKEH_MSPlot, PeakMapPlot):
"""

# NOTE: canvas is only used in matplotlib backend
def create_main_plot(self, canvas=None):
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.
Comment on lines +560 to +561
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put these under a note

Suggested change
- 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.plotting import figure
from bokeh.models import ColorBar, LinearColorMapper
Comment on lines +563 to +564
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these imports here because they're only strictly for this method?


if not self.plot_3d:
# ✅ Apply log scaling **only for PeakMap color scale**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this comment

Suggested change
# ✅ 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]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually get used anywhere? It doesn't seem like it?


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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this comment

Suggested change
# ✅ 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
Comment on lines +557 to +582
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 variable

The 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.

Suggested change
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)

Comment on lines +557 to +582
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the indentation of this method.


return fig


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra space


def create_x_axis_plot(self):
"""
Creates the X-axis marginal histogram plot.
- Uses `z_original` to ensure raw intensity values are displayed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this as well similar to the suggestion above.

"""
x_fig = super().create_x_axis_plot()

# Modify plot
# ✅ Ensure marginal plots use raw intensity values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ✅ Ensure marginal plots use raw intensity values

x_fig.x_range = self.main_fig.x_range
x_fig.width = self.x_plot_config.width
x_fig.xaxis.visible = False

# ✅ Use `z_original` for raw intensity in histograms
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ✅ Use `z_original` for raw intensity in histograms

if "z_original" in self.data.columns:
x_fig.circle(self.data[self.x], self.data["z_original"], size=5, color="gray")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we add a circle glyph for x marginal plots for the original z value?


return x_fig


def create_y_axis_plot(self):
"""
Creates the Y-axis marginal histogram plot.
- Uses `z_original` to ensure raw intensity values are displayed.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this similar to suggestion above

"""
y_fig = super().create_y_axis_plot()

# Modify plot
# ✅ Ensure marginal plots use raw intensity values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ✅ Ensure marginal plots use raw intensity values

y_fig.y_range = self.main_fig.y_range
y_fig.height = self.y_plot_config.height
y_fig.legend.orientation = self.y_plot_config.legend_config.orientation
y_fig.x_range.flipped = True

# ✅ Use `z_original` for raw intensity in histograms
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# ✅ Use `z_original` for raw intensity in histograms

if "z_original" in self.data.columns:
y_fig.circle(self.data["z_original"], self.data[self.y], size=5, color="gray")
Comment on lines +620 to +621
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here as above


return y_fig


def combine_plots(self, main_fig, x_fig, y_fig):
# Modify the main plot
main_fig.yaxis.visible = False
Expand Down
61 changes: 44 additions & 17 deletions pyopenms_viz/_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,13 @@ class PeakMapConfig(ScatterConfig):
title (str): Title of the plot. Default is "PeakMap".
x_plot_config (ChromatogramConfig | SpectrumConfig): Configuration for the X-axis marginal plot. Set in post-init.
y_plot_config (ChromatogramConfig | SpectrumConfig): Configuration for the Y-axis marginal plot. Set in post-init.

# FIXED BEHAVIOR: Log scaling always applied to colors, not intensity
z_log_scale_colors: bool = True # Always log scale colors
z_log_scale_intensity: bool = False # Always keep raw intensities
Comment on lines +414 to +417
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more information here to explain the usage and purpose of these add params.

"""


@staticmethod
def marginal_config_factory(kind):
if kind == "chromatogram":
Expand All @@ -437,10 +442,48 @@ def marginal_config_factory(kind):

def __post_init__(self):
super().__post_init__()
# initial marginal configs

# Set default marginal configurations
self.y_plot_config = PeakMapConfig.marginal_config_factory(self.y_kind)
self.x_plot_config = PeakMapConfig.marginal_config_factory(self.x_kind)

# ✅ Ensure marginals always use raw intensity values
self.y_plot_config.z_log_scale = False
self.x_plot_config.z_log_scale = False

# ✅ Update labels for proper visualization
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 = ""
Comment on lines +455 to +471
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 = 0

Also applies to: 487-512

Comment on lines +450 to +471
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be changed a lot from what it originally was? Any reason why?


# ✅ Ensure only colors are log-scaled
if not self.fill_by_z:
self.z = None

self.annotation_data = (
None if self.annotation_data is None else self.annotation_data.copy()
)
Comment on lines +477 to +479
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems duplicated/done below as well?



# Ensure marginals always use raw intensity values
self.y_plot_config.z_log_scale = False
self.x_plot_config.z_log_scale = False


# update y-axis labels and positioning to defaults
self.y_plot_config.xlabel = self.zlabel
self.y_plot_config.ylabel = self.ylabel
Expand Down Expand Up @@ -510,19 +553,3 @@ def bokeh_line_dash_mapper(bokeh_dash, target_library="plotly"):
# If it's already a valid Plotly dash type, return it as is
if bokeh_dash in plotly_mapper.values():
return bokeh_dash
# Otherwise, map from Bokeh to Plotly
return plotly_mapper.get(bokeh_dash, "solid")
elif isinstance(bokeh_dash, list):
return " ".join(f"{num}px" for num in bokeh_dash)
elif target_library.lower() == "matplotlib":
if isinstance(bokeh_dash, str):
# If it's already a valid Matplotlib dash type, return it as is
if bokeh_dash in matplotlib_mapper.values():
return bokeh_dash
# Otherwise, map from Bokeh to Matplotlib
return matplotlib_mapper.get(bokeh_dash, "-")
elif isinstance(bokeh_dash, list):
return (None, tuple(bokeh_dash))

# Default return if target_library is not recognized or bokeh_dash is neither string nor list
return "solid" if target_library.lower() == "plotly" else "-"
Comment on lines -513 to -528
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do no longer need this?

89 changes: 48 additions & 41 deletions pyopenms_viz/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -1118,54 +1118,41 @@ def known_columns(self) -> List[str]:
def _configClass(self):
return PeakMapConfig

def __init__(self, data, **kwargs) -> None:
import pandas as pd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a pandas import here?


def __init__(self, data, z_log_scale=False, **kwargs) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't z_log_scale already be part of the PeakMapConfig, so why do we pass it here?

super().__init__(data, **kwargs)
self.z_log_scale = z_log_scale

# ✅ Store original intensity values before applying log transformation
if hasattr(self, "z") and isinstance(self.z, str):
if isinstance(self.data, pd.DataFrame) and self.z in self.data.columns:
self.z_original = self.data[self.z].copy() # Ensure it's a Series before copying
else:
self.z_original = None # Handle cases where z is not a valid column
else:
self.z_original = None # Handle unexpected cases

self._check_and_aggregate_duplicates()
self.prepare_data()
self.plot()

def prepare_data(self):
# Convert intensity values to relative intensity if required
if self.relative_intensity and self.z is not None:
self.data[self.z] = self.data[self.z] / max(self.data[self.z]) * 100

# Bin peaks if required
if self.bin_peaks == True or (
self.data.shape[0] > self.num_x_bins * self.num_y_bins
and self.bin_peaks == "auto"
):
self.data[self.x] = cut(self.data[self.x], bins=self.num_x_bins)
self.data[self.y] = cut(self.data[self.y], bins=self.num_y_bins)
if self.z is not None:
if self.by is not None:
# Group by x, y and by columns and calculate the mean intensity within each bin
self.data = (
self.data.groupby([self.x, self.y, self.by], observed=True)
.agg({self.z: self.aggregation_method})
.reset_index()
)
else:
# Group by x and y bins and calculate the mean intensity within each bin
self.data = (
self.data.groupby([self.x, self.y], observed=True)
.agg({self.z: "mean"})
.reset_index()
)
self.data[self.x] = (
self.data[self.x].apply(lambda interval: interval.mid).astype(float)
)
self.data[self.y] = (
self.data[self.y].apply(lambda interval: interval.mid).astype(float)
)
self.data = self.data.fillna(0)
Comment on lines -1128 to -1160
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to remove the ability to plot relative intensity / binning the data.

def prepare_data(self):
"""
Prepares the dataset for plotting.
- Ensures log transformation applies only to colors.
- Keeps original intensity values (`z_original`) for marginal histograms.
"""
if hasattr(self, "z"):
self.data["z_original"] = self.data[self.z] # Save original intensity

# Log intensity scale
# ✅ 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])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does self.data["color_intensity"] even get used anywhere in the backends plotting logic?

else:
self.data["color_intensity"] = self.data[self.z]

# 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)
Comment on lines -1166 to -1168
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 plot(self):

Expand Down Expand Up @@ -1201,8 +1188,28 @@ def create_main_plot(self, canvas=None):
pass

# by default the main plot with marginals is plotted the same way as the main plot unless otherwise specified
def create_main_plot_marginals(self, canvas=None):
return self.create_main_plot(canvas)
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
Comment on lines +1191 to +1211
Copy link
Collaborator

@singjc singjc Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



@abstractmethod
def create_x_axis_plot(self, canvas=None) -> "figure":
Expand Down
Loading
Loading