Added panning and zooming options. #195
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant V as view_peak_map()
participant P as Plotly Figure
participant S as Streamlit Renderer
U->>V: Request peak map and TIC view
V->>P: Build combined figure with box selection, range slider, and updated layout
P-->>V: Return configured figure
V->>S: Render figure using st.plotly_chart
Assessment against linked issues
Poem
✨ 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 (
|
|
Thanks for opening this PR. Really cool, I actually didnt know about the rangeslider. While I think it is a bit overkill for the plot you applied it to, could you check if its possible to use it for the peak map (the 2D-Plot). What we would be interested in is displaying an additional dimension, which sums up all the intensity values for a given retention time (usually called TIC). |
|
Can you display the TIC as well or is it not working? |
|
|
@t0mdavid-m do the changes look okay? |
|
No, I think @t0mdavid-m means to add a marginal plot to the 2D plot at x and y axis that represent the sum of intensities (i.e. 3rd dimension/color) as a line plot. |
|
@jpfeuffer |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/view.py (1)
308-308: Consider adding container configuration options.The 3D plot is displayed with
use_container_width=Truebut without any other configuration options. Consider using theshow_figfunction as done for the 2D plot to ensure consistent behavior and download options.-st.plotly_chart(peak_map_3D, use_container_width=True) +show_fig( + peak_map_3D, + f"peak_map_3D_{st.session_state.view_selected_file}", + True +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/view.py(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/view.py (1)
src/common/common.py (1) (1)
show_fig(517-581)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-openms
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: build
🔇 Additional comments (10)
src/view.py (10)
10-10: Appropriate import addition for subplots functionality.The import of
make_subplotsis necessary for creating the combined figure with peak map and TIC visualization in the enhanced implementation.
175-184: Well-implemented box selection filtering.The box selection filtering allows users to zoom into specific regions of the peak map, which aligns with the PR objective of enhancing zooming capabilities. The implementation correctly filters the data based on the selected region's boundaries.
190-191: Good improvement with explicit axis labels.Replacing the title parameter with explicit axis labels improves clarity and follows best practices for scientific visualization.
199-218: Well-implemented TIC visualization.The implementation of the Total Ion Chromatogram (TIC) directly addresses the suggestion in the PR comments to display an additional dimension that aggregates all intensity values for a given retention time. The code correctly:
- Groups and sums the intensity values by retention time
- Creates a separate figure with appropriate styling
- Configures clear axis labels
This implementation meets the requirements mentioned in the PR comments.
220-262: Excellent subplots implementation for combined visualization.The implementation of the combined figure with the peak map and TIC is well-structured and follows best practices:
- Appropriate use of
make_subplotswith shared x-axes- Good proportion allocation between peak map (70%) and TIC (30%)
- Well-configured range slider for zooming/panning
- Clear title and axis labels
This implementation effectively addresses the main objective of the PR to add panning and zooming options while incorporating the TIC visualization as suggested in the PR comments.
268-270: Clear user guidance for the new functionality.The updated hint provides clear instructions for users on how to utilize the new zooming functionality, which enhances the user experience.
287-289: Improved 3D plot axis labeling.The addition of explicit axis labels for the 3D plot improves clarity and consistency with the 2D visualization.
299-306: Enhanced 3D visualization configuration.The explicit scene configuration with proper axis titles and orbit drag mode significantly improves the 3D visualization's usability and clarity. This complements the zooming and panning features added to the 2D visualization.
403-422: Excellent chromatogram plot enhancements.The improvements to the chromatogram plot are well-implemented:
- The range slider provides intuitive zoom control
- The zoom buttons offer convenient navigation options
- The margin and height adjustments improve visibility
- The hover mode and modebar configurations enhance user interaction
These enhancements align with the PR's objective to improve zooming and panning capabilities.
424-425:Details
❓ Verification inconclusive
Improved plot display method.
Replacing the
show_figfunction with directst.plotly_chartanduse_container_width=Trueensures the chromatogram fills the available space, improving visibility.However, this change loses the download functionality and consistent configuration provided by the
show_figfunction seen in line 271. Consider if this tradeoff is intentional.
🏁 Script executed:
#!/bin/bash # Check how show_fig is used throughout the codebase to verify consistency grep -n "show_fig" --include="*.py" -r .Length of output: 632
Action Required: Verify Intentional Loss of Download Functionality in Plot Display
The changes in
src/view.pyat lines 424–425 now directly callst.plotly_chart(fig, use_container_width=True), which effectively utilizes the full available width for the chromatogram. However, as verified from the repository-wide usage ofshow_fig(e.g., insrc/view.pyat line 271,src/Workflow.py, and others), theshow_figfunction also provides download functionality and enforces consistent configuration. Please confirm whether droppingshow_figfor this particular plot is an intentional tradeoff. If the additional features are still desired for chromatograms, consider either reintegratingshow_figor replicating its configuration and download capabilities.
|
Yep nice. But: |
@jpfeuffer @t0mdavid-m |
|
How does the interactivity with the second option b2 look like? How do you select a range? Can you make a video? By the way, can you invert the y axis of the TIC such that y=0 is directly attached to the 2d plot and high values point away from the 2d plot. I think this is more of a convention. |
sure, kindly check Untitled.video.-.Made.with.Clipchamp.2.mp4 |
|
Thanks! I'm looking for a horizontal range selection on the TIC subplot. Is that doable as well? |
Untitled.video.-.Made.with.Clipchamp.3.mp4something like this? |
|
or we can go for this design |
|
No, I just want |
done. Is this what you meant? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/view.py (3)
311-311: Consider using the show_fig function for consistencyThe 3D peak map is displayed using
st.plotly_chart()directly while other visualizations use theshow_fig()helper function.For better consistency, consider using the
show_fig()function here as well:- st.plotly_chart(peak_map_3D, use_container_width=True) + show_fig(peak_map_3D, f"peak_map_3D_{st.session_state.view_selected_file}", True)This would also ensure that the same configuration settings (like download options) are applied consistently across all visualizations.
427-428: Consider using show_fig for consistent handlingSimilar to the 3D peak map, this visualization uses
st.plotly_chart()directly instead of theshow_fig()helper function.For consistency with other visualizations:
- st.plotly_chart(fig, use_container_width=True) + show_fig(fig, f"chromatogram_{st.session_state.view_selected_file}", True)This would provide consistent download options and configuration across all visualizations.
228-228: Fix indentation in selectdirection parameterThere's a minor indentation issue with the
selectdirectionparameter.- selectdirection = "h" + selectdirection="h"This would maintain consistency with the surrounding code style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/view.py(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/view.py (1)
src/common/common.py (1) (1)
show_fig(517-581)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-openms
- GitHub Check: build-full-app
- GitHub Check: build-simple-app
🔇 Additional comments (9)
src/view.py (9)
10-10: Good addition of required importThe import
from plotly.subplots import make_subplotsis appropriately added to support the new functionality for creating combined figures with subplots.
174-183: Well-implemented box selection filteringThe box selection filtering mechanism correctly extracts and applies the selection boundaries to filter the data. This implementation properly handles the x (retention time) and y (m/z) coordinates to focus the view on the selected region.
184-197: Appropriate axis labeling improvementGood improvement replacing the generic title with explicit axis labels "Retention Time (s)" and "m/z". This enhances the clarity and professionalism of the visualization.
199-228: Well-crafted TIC plot implementationThe TIC plot implementation correctly:
- Aggregates data by retention time
- Creates a clear visual representation with appropriate styling
- Inverts the y-axis as requested in the PR comments
- Enables horizontal selection mode for better user interaction
The separation of this plot from the peak map implementation follows good design principles.
230-264: Excellent combined figure implementationThe combined figure implementation using
make_subplotseffectively:
- Creates a shared x-axis between plots for synchronized navigation
- Uses appropriate proportions (70/30 split) for the peak map and TIC
- Maintains consistent styling and proper axis labeling
- Includes clear title formatting with good positioning
This implementation aligns perfectly with the requested feature in the PR objectives.
271-273: Clear user guidanceThe updated info message provides clear instructions for users on how to interact with the new selection functionality.
290-292: Proper axis labeling for 3D viewGood addition of explicit axis labels for the 3D peak map visualization, which improves consistency with the 2D visualization.
302-310: Enhanced 3D visualization controlsThe layout update for the 3D peak map correctly sets axis titles and enables orbit drag mode, which significantly improves the user interaction experience with the 3D visualization.
406-425: Great interactivity enhancements for chromatogram plotThe chromatogram plot now features excellent interactive elements:
- Range slider for easy navigation along the x-axis
- Zoom buttons for quick time range selection
- Improved layout with better height and margin settings
- Enhanced hover mode and toolbar configuration
These changes significantly improve the user experience when exploring chromatogram data.
Untitled.video.-.Made.with.Clipchamp.5.mp4 |
|
😄 I mean horizontal range selections, aka horizontal brushing, aka dragging the mouse from left to right. I just wanna see it in the video. The code looks about right. By the way, dragging the whole plot up and down should NOT be possible. Please disable if possible. |
sure, I'll disable that. However, I have shown the dragging from left to right in the plot. Isn't that what you meant 😅 ? |
Untitled.video.-.Made.with.Clipchamp.6.mp4@jpfeuffer kindly have a look at this now |
|
Haha yes I mean the selection you did after a few seconds in the video. Seems to work. For my personal taste, it would be nice if you could not drag the plots outside of their limits. |
I tried doing that, however the thing is that the functionality is only supported for javascript. Currently in plotly for python, if we are allowing horizontal brushing in the plot, we can't stop the user from brushing past a certain point. |
|
I have pushed the changes, unfortunately the clamping of the x axis can't be implemented in plotly's python version. However, rest of the things have been implemented. Kindly check @jpfeuffer |
|
Can you elaborate why? The function is clearly there. |
minallowed and maxallowed do not prevent zooming or selection outside the range They do not restrict zooming and panning beyond the range. They also do not affect box selection behavior in Plotly plots. Now, since we are not implementing the rangeslider but rather using normal horizontal brushing, we can't stop the user from going beyond the range. |
|
I cannot reproduce this. |
Untitled.video.-.Made.with.Clipchamp.7.mp4
so I applied the maxallowed and minallowed thing on the plot, the issue is exactly as you mentioned. the plot resists going beyond the minimum and maximum range, but if we forcibly stretch it, then it glitches out. |
|
and one more thing I can do is, I can disable horizontal brushing on the TIC plot, and only keep it for the peak map. Should I do that? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/view.py(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/view.py (1)
src/common/common.py (1)
show_fig(517-581)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-openms
- GitHub Check: build-simple-app
🔇 Additional comments (10)
src/view.py (10)
10-10: Import of make_subplots - Good addition to support the new visualization.The import of
make_subplotsfromplotly.subplotsis necessary for creating the combined figure with the peak map and TIC plot.
178-181: Good constraint implementation for RT values.The implementation of constraints to ensure RT values stay within reasonable bounds (min: 0, max: 1500) is a good practice to prevent outliers from affecting visualization.
188-190: Improved axis labeling with proper units.Replacing the title parameter with explicit axis labels provides better clarity for users interpreting the data.
197-209: Good implementation of the TIC plot.The Total Ion Chromatogram (TIC) implementation effectively aggregates intensity values by retention time, providing users with an additional dimension of data analysis.
210-226: TIC layout configuration is properly set up.The layout configuration for the TIC plot is well designed with:
- Appropriate height (200px) for a secondary plot
- Proper margin settings
- Y-axis correctly inverted as requested in PR comments
- Fixed range to prevent y-axis zooming which could distort the visualization
This implementation aligns well with the requested changes from the PR discussion.
228-241: Well-structured subplot creation.The implementation of the combined figure using
make_subplotsis clean and efficient:
- Proper row heights allocation (70% for peak map, 30% for TIC)
- Shared x-axes ensuring synchronized zooming/panning
- Appropriate vertical spacing
264-266: Updated user guidance is informative.The updated information message clearly guides users on how to interact with the visualization, mentioning both the selection functionality and how to reset it.
267-271: Proper usage of the show_fig function with the combined figure.The
show_figfunction is correctly used to display the combined figure with appropriate parameters for the selection state key.
398-417: Enhanced BPC-TIC plot with interactive controls.The improvements to the BPC-TIC plot significantly enhance user experience:
- The range slider provides an excellent overview of the entire dataset
- Zoom buttons offer quick access to common time ranges
- Increased height (700px) improves visualization of complex chromatograms
- Unified hover tooltip makes it easier to compare values across different traces
These enhancements align perfectly with the PR objective of improving interactivity.
419-420: Good implementation of full-width display.Using
use_container_width=Trueensures the visualization adapts to the available screen space, maximizing the viewing area.
| peak_map_3D.update_layout( | ||
| scene=dict( | ||
| xaxis=dict(title="Retention Time (s)", minallowed="0", maxallowed="1000"), | ||
| yaxis=dict(title="m/z", fixedrange=True), | ||
| zaxis=dict(title="Intensity"), | ||
| dragmode="orbit" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
3D peak map layout alignment with 2D plot.
The 3D peak map layout is correctly configured to maintain consistency with the 2D visualization, however it has the same issue with minallowed and maxallowed properties:
- xaxis=dict(title="Retention Time (s)", minallowed="0", maxallowed="1000"),
+ xaxis=dict(title="Retention Time (s)", range=[0, 1000]),Setting dragmode="orbit" is a good choice for 3D visualization as it provides more intuitive navigation.
📝 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.
| peak_map_3D.update_layout( | |
| scene=dict( | |
| xaxis=dict(title="Retention Time (s)", minallowed="0", maxallowed="1000"), | |
| yaxis=dict(title="m/z", fixedrange=True), | |
| zaxis=dict(title="Intensity"), | |
| dragmode="orbit" | |
| ) | |
| ) | |
| peak_map_3D.update_layout( | |
| scene=dict( | |
| xaxis=dict(title="Retention Time (s)", range=[0, 1000]), | |
| yaxis=dict(title="m/z", fixedrange=True), | |
| zaxis=dict(title="Intensity"), | |
| dragmode="orbit" | |
| ) | |
| ) |
| combined_fig.update_layout( | ||
| template="simple_white", | ||
| dragmode="zoom", | ||
| xaxis=dict(title="Retention Time (s)", showgrid=False, minallowed="0", maxallowed="1000"), | ||
| yaxis=dict(title="m/z", fixedrange=True), | ||
| yaxis2=dict(title="TIC", autorange="reversed", fixedrange=True), | ||
| height=850, | ||
| margin=dict(t=100, b=100), | ||
| title=dict( | ||
| text=st.session_state.view_selected_file, | ||
| x=0.5, | ||
| y=0.99, | ||
| xanchor="center", | ||
| yanchor="top", | ||
| font=dict(size=18, family="Arial, sans-serif") | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Combined figure layout could be improved.
While the layout configuration is generally good, there's an issue with the minallowed and maxallowed properties:
- xaxis=dict(title="Retention Time (s)", showgrid=False, minallowed="0", maxallowed="1000"),
+ xaxis=dict(title="Retention Time (s)", showgrid=False, range=[0, 1000]),The minallowed and maxallowed properties define input constraints but don't limit the visible range. The range property would better ensure the visualization starts with the desired range. Also, the values are passed as strings instead of numbers, which is inconsistent with Plotly conventions.
📝 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.
| combined_fig.update_layout( | |
| template="simple_white", | |
| dragmode="zoom", | |
| xaxis=dict(title="Retention Time (s)", showgrid=False, minallowed="0", maxallowed="1000"), | |
| yaxis=dict(title="m/z", fixedrange=True), | |
| yaxis2=dict(title="TIC", autorange="reversed", fixedrange=True), | |
| height=850, | |
| margin=dict(t=100, b=100), | |
| title=dict( | |
| text=st.session_state.view_selected_file, | |
| x=0.5, | |
| y=0.99, | |
| xanchor="center", | |
| yanchor="top", | |
| font=dict(size=18, family="Arial, sans-serif") | |
| ) | |
| ) | |
| combined_fig.update_layout( | |
| template="simple_white", | |
| dragmode="zoom", | |
| - xaxis=dict(title="Retention Time (s)", showgrid=False, minallowed="0", maxallowed="1000"), | |
| + xaxis=dict(title="Retention Time (s)", showgrid=False, range=[0, 1000]), | |
| yaxis=dict(title="m/z", fixedrange=True), | |
| yaxis2=dict(title="TIC", autorange="reversed", fixedrange=True), | |
| height=850, | |
| margin=dict(t=100, b=100), | |
| title=dict( | |
| text=st.session_state.view_selected_file, | |
| x=0.5, | |
| y=0.99, | |
| xanchor="center", | |
| yanchor="top", | |
| font=dict(size=18, family="Arial, sans-serif") | |
| ) | |
| ) |
|
@jpfeuffer does this look good to go now 😅 ? |
|
I guess we will have to live with the glitch. The horizontal brushing on the TIC is nice actually, we should leave it. @t0mdavid-m Any feedback before merge? |
|
@t0mdavid-m , kindly review the changes made 😄 |
|
That looks really cool! One final idea is if we could make the TIC plot a bit smaller (y-axis)? Otherwise this is ready to merge from my side. |
done. Kindly check |
|
Just wanted to add that the |
you mean something like this? |
|
@subCode321 could you check if the plot you shared previously can also be replicated using pyopenms-viz as @singjc described? |
Which one? The red TIC one? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/view.py (3)
211-229: 🛠️ Refactor suggestionIssue with range constraints in TIC layout.
The
minallowedandmaxallowedproperties define input constraints but don't limit the visible range. Additionally, they're passed as strings instead of numbers.- xaxis=dict( - title="Retention Time (s)", - rangeslider=dict(visible=False), - showgrid=False, - fixedrange=True, - minallowed="0", - maxallowed="1000" - ), + xaxis=dict( + title="Retention Time (s)", + rangeslider=dict(visible=False), + showgrid=False, + fixedrange=True, + range=[0, 1000] + ),
245-261: 🛠️ Refactor suggestionCombined figure layout could be improved.
While the layout configuration is generally good, there's an issue with the
minallowedandmaxallowedproperties.- xaxis=dict(title="Retention Time (s)", showgrid=False, minallowed="0", maxallowed="1000"), + xaxis=dict(title="Retention Time (s)", showgrid=False, range=[0, 1000]),
297-304: 🛠️ Refactor suggestion3D peak map layout alignment with 2D plot.
The 3D peak map layout is correctly configured to maintain consistency with the 2D visualization, however it has the same issue with
minallowedandmaxallowedproperties.- xaxis=dict(title="Retention Time (s)", minallowed="0", maxallowed="1000"), + xaxis=dict(title="Retention Time (s)", range=[0, 1000]),Setting
dragmode="orbit"is a good choice for 3D visualization as it provides more intuitive navigation.
🧹 Nitpick comments (1)
src/view.py (1)
306-306: Consider using show_fig for consistent behavior.Using
st.plotly_chartdirectly for the 3D plot bypasses theshow_figfunction used for other plots, which could lead to inconsistent behavior regarding download options and toolbar customization.- st.plotly_chart(peak_map_3D, use_container_width=True) + show_fig( + peak_map_3D, + f"peak_map_3D_{st.session_state.view_selected_file}", + container_width=True + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/view.py(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/view.py (1)
src/common/common.py (1)
show_fig(517-581)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-openms
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
🔇 Additional comments (9)
src/view.py (9)
10-10: Good addition for subplot functionality.Adding the
make_subplotsimport is necessary for creating the combined figure with peak map and TIC plots.
178-181: Great data validation for RT boundaries.The addition of constraints to limit retention time values between 0 and 1500 prevents filtering beyond valid ranges, ensuring data integrity and preventing potential visualization issues.
188-190: Improved axis labeling approach.Using explicit
xlabelandylabelparameters instead of embedding this information in a title is a better practice for plot readability and accessibility.
197-209: Good implementation of Total Ion Chromatogram (TIC).The TIC aggregation logic correctly sums intensities by retention time and creates an appropriate chromatogram visualization with proper axis labels.
231-243: Well-structured subplot implementation.The subplot creation with appropriate row heights and shared x-axes is well-implemented. The traces are correctly added to their respective subplot positions.
267-269: Clear user instructions.The updated info message clearly instructs users on how to interact with the TIC plot, enhancing usability.
271-274: Good integration with existing functionality.The combined figure is properly passed to the
show_figfunction with appropriate parameters for selection handling.
286-288: Consistent axis labeling in 3D plot.Using explicit axis labels in the 3D peak map maintains consistency with the 2D visualization.
401-423: Excellent interactive enhancements for chromatogram visualization.The addition of range slider, zoom buttons, and improved layout configuration significantly enhances the user experience. The increased height provides better visualization of the data, and the unified hover mode makes it easier to compare values across different traces.
The range slider and zoom buttons are particularly helpful for navigating through retention time ranges, aligning perfectly with the PR's objective of adding panning and zooming capabilities.
|
@t0mdavid-m , I have done the necessary changes. |
|
@t0mdavid-m , is the PR good to go now? |
|
Could you doublecheck if the current version works on your end? On my end the TIC is not being displayed anymore. |
|
@t0mdavid-m yes I can see the plot |
|
@t0mdavid-m I tried again on another laptop and I'm able to see the TIC plot. Can you please check once again from your side ? the branch is |
|
@t0mdavid-m did you check? 😅 |
4 participants
One can now view the chromatogram plot by zooming in and panning it. I have used plotly's inbuilt features to implement the same.
Fixes #194
Recording.2025-03-21.001924.mp4
Summary by CodeRabbit