-
Notifications
You must be signed in to change notification settings - Fork 22
Added a comparison panel to the CycleBlock for displaying multiple files for comparison #1353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a comparison panel to the CycleBlock for displaying multiple files for comparison. The changes replace the binary toggle between single and multi-file modes with a three-option button group and implement proper comparison functionality with distinct colormaps and legends for each file.
- Replaced binary mode toggle with three-mode selection: Single File, Multi File Stitch, and Comparison
- Added comparison plotting functionality with unique colormaps per file and proper legends
- Enhanced hover tooltips to include filename information in comparison mode
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
webapp/src/components/datablocks/CycleBlock.vue | Replaces binary toggle with three-mode button group and updates logic to handle comparison mode |
pydatalab/src/pydatalab/bokeh_plots.py | Adds comparison mode support with unique colormaps per file and enhanced legends/tooltips |
pydatalab/src/pydatalab/apps/echem/utils.py | Adds .copy() calls to prevent DataFrame mutation warnings |
pydatalab/src/pydatalab/apps/echem/blocks.py | Implements comparison mode data loading and processing logic |
Comments suppressed due to low confidence (1)
pydatalab/src/pydatalab/apps/echem/blocks.py:1
- The variable 'cycle_summary' is not defined in this scope. This appears to be a leftover from the old single DataFrame approach. Should check 'cycle_summary_dfs' instead.
import os
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
hover_source = ColumnDataSource( | ||
{ | ||
"full cycle": cycle_summary["cycle index"], | ||
"filename": cycle_summary["filename"], | ||
"charge": cycle_summary[charge_col], | ||
"discharge": cycle_summary[discharge_col], | ||
} | ||
) |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hover source uses 'cycle index' from cycle_summary but maps it to 'full cycle' key. This is inconsistent with other parts of the code that use 'full cycle' column. Should use cycle_summary['full cycle'] instead of cycle_summary['cycle index'].
Copilot uses AI. Check for mistakes.
datalab
|
Project |
datalab
|
Branch Review |
bes/echem_analysis_panel
|
Run status |
|
Run duration | 07m 09s |
Commit |
|
Committer | Ben Smith |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
336
|
View all changes introduced in this branch ↗︎ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1353 +/- ##
==========================================
- Coverage 80.10% 79.79% -0.32%
==========================================
Files 70 70
Lines 4755 4825 +70
==========================================
+ Hits 3809 3850 +41
- Misses 946 975 +29
🚀 New features to boost your workflow:
|
This is a cool step forward. I would like to maintain a differentiation in the UI between the one (or multiple stiched) data sets that the block is keeping track of, and the one or more samples that are being compared to it. So, instead of having it as a tabbed option, I think we should have an option on the main tab to add additional comparisons to the graph only. The block itself is in charge of keeping track of the main experiment along with any experimental details/metadata, and displaying additional comparisons is a helpful additional feature. This is the same thing we discussed with the XRD block, which currently has a similar issue. |
I've split up the "main" block files and the comparison files into different file selectors. They are also stored as different fields: The UI could use some work still but is this approach a good response to your comments @jdbocarsly ? The next entension is I think a sample selector rather than a file selector so you're grabbing files from other samples or blocks like we discussed in the meeting ![]() |
yeah, that's exactly what I was imagining! And yes, agree that some way of grabbing the trace from another sample would be good (and that way you can normalize by the correct mass, ideally). By the way, while you are at this-- would it be easy to add a second x- axis to the echem capacity vs voltage plot, which has capacity in units of Li (or Na) transferred per mol? We store the molar mass if the user has put in a formula, so I think in principle this should be pretty straightforward. For generality, I would probably call it something like "e– transferred/mol". We would often express it as "x in LixCoO2," but that gets hard to generalize since there are different starting points. This could be in a separate PR if that is cleaner. |
Yeah that should be doable, but yes might be a future PR |
4b00aac
to
328673f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
pydatalab/src/pydatalab/bokeh_plots.py:844
- Variable
cycle_summary
is undefined in this context. It should becycle_summary_dfs
to match the updated function signature.
if cycle_summary is not None:
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
328673f
to
e770e89
Compare
…with new logic. Added check to convert single dfs to a list of dfs just incase in plotting function
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…sing "full cycle" not "cycle index"
…ade comparison file list collapsible
… for ease of reuse in other blocks
…es. Only the final saved state persists across reloads
- Refactor CollapsibleComparisonFileSelect to use Vue's Transition component for cleaner animation handling - Improve code readability and structure in both CycleBlock and CollapsibleComparisonFileSelect components
e770e89
to
bf98500
Compare
Adds a panel for comparison - handles legends and different colourmaps for different files etc.
Future add-ons