Adding echem pathway to xrd insitu block #67
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds electrochemical (echem) pathway support to the XRD in-situ block, allowing users to visualize XRD data against voltage/time instead of just temperature. It also updates the UV-Vis block to handle multiple electrochemical files.
- Adds
time_series_sourceparameter to switch between "log" (temperature) and "echem" (electrochemical) data sources - Enhances echem_utils.py to handle multiple electrochemical files with concatenation and deduplication
- Updates plotting functions to support both pathways with appropriate axis labels and data handling
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_inisitu_xrd_block.py | Sets time_series_source to "log" for testing the temperature pathway |
| src/datalab_app_plugin_insitu/plotting.py | Adds conditional logic for echem vs log data handling in XRD plot preparation |
| src/datalab_app_plugin_insitu/echem_utils.py | Enhanced to handle multiple echem files with proper validation and timestamp processing |
| src/datalab_app_plugin_insitu/blocks.py | Improved max pooling implementation with better padding and error handling |
| src/datalab_app_plugin_insitu/apps/xrd/utils.py | Major updates to support echem pathway including timestamp matching and data merging |
| src/datalab_app_plugin_insitu/apps/xrd/blocks.py | Converts plotting_label_dict to property for dynamic labels based on data source |
| README.md | Updated documentation to describe both temperature and electrochemical pathways |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if self.data.get("time_series_source") == "log": | ||
| for folder in required_folders: | ||
| if not self.data.get(folder): | ||
| return | ||
| elif self.data.get("time_series_source") == "echem": | ||
| for folder in required_folders: | ||
| if not self.data.get(folder): | ||
| return |
There was a problem hiding this comment.
The conditional logic for checking required folders is duplicated. This can be simplified to a single loop since both conditions perform the same validation.
| if self.data.get("time_series_source") == "log": | |
| for folder in required_folders: | |
| if not self.data.get(folder): | |
| return | |
| elif self.data.get("time_series_source") == "echem": | |
| for folder in required_folders: | |
| if not self.data.get(folder): | |
| return | |
| for folder in required_folders: | |
| if not self.data.get(folder): | |
| return |
Copilot uses AI. Check for mistakes.
| for ext in accepted_extensions: | ||
| echem_files.extend(echem_folder.glob(f"*{ext}")) | ||
|
|
||
| # Sanity check to remove any duplicate files |
There was a problem hiding this comment.
[nitpick] The comment mentions removing duplicates, but dict.fromkeys() preserves order while removing duplicates. Consider using set() if order doesn't matter, or clarify the comment to mention order preservation.
| # Sanity check to remove any duplicate files | |
| # Remove duplicate files while preserving order |
Copilot uses AI. Check for mistakes.
| if set(new_two_theta_values) != set(two_theta): | ||
| missing_values = set(two_theta) - set(new_two_theta_values) | ||
| LOGGER.warning( | ||
| f"Inconsistent 2θ values found in file {file}: {missing_values}." | ||
| ) |
There was a problem hiding this comment.
Converting arrays to sets for comparison is inefficient for large arrays. Consider using np.array_equal() or np.allclose() for floating-point comparisons, which would be more appropriate for 2θ values.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
|
The state of play here is that i think everything works - but the code is messy and still haven't unified NMR and UV-Vis/XRD plotting modules. So there will be another PR on top of this one where everything is tidied and plotting mofules are unified. |
Adds the echem pathway for the XRD block (and updates it for the UV-Vis block to be able to handle multiple files). Paired with datalab-org/datalab#1287 for the block itself. Currently the test data I'm not sure can be added to GitHub pending permission from the owner so will send on slack