Open
Conversation
- Added ylim argument to plotting functions - Enabled independent tick spacing for axes - Implemented grid plotting for 2D Bruker and DMFit data - Improved type safety and error handling - Refactored some code for clarity
Breaking Changes (API renames) - colors → color in bruker2d, bruker2d_grid, dmfit2d, dmfit2d_grid - proj_colors → proj_color in all 2D plot functions - homo → homonuclear in bruker2d, bruker2d_grid - diag → diagonal in all 2D plot functions - labels → titles in dmfit1d_grid (for subplot titles) Bug Fixes - read_nmr() now accepts tags="single_string" (no longer requires a list for one path) - All plot functions now warn on unrecognized kwargs instead of silently ignoring typos - Standardized save logic across all 9 plot functions (consistent defaults, no crashes) - Fixed __init__.py exports (all public functions now importable) - Standardized projection keys to "f1"/"f2" in Bruker 2D plots Internal Improvements - Extracted shared helpers into _helpers.py (save, tick spacing, nuclei labels, per-subplot limits) - dmfit1d now uses global DEFAULTS dict like all other functions - All plot functions accept spectrum dicts (unified input types) - Renamed tests/tests_io.py → test_io.py (was not being collected by pytest) Docs - Documented normalize parameters - Documented xlim/ylim list-of-tuples support in all grid functions - Updated user guide notebooks with new parameter names
There was a problem hiding this comment.
Pull request overview
This PR implements version 0.2.2 of SpinPlots, addressing issues #29 and #31 with significant enhancements to plotting functionality and API consistency.
Changes:
- Added grid layouts for all plot types (
bruker2d_grid,dmfit1d_grid,dmfit2d_grid) - Unified parameter naming across all plot functions for consistency
- Added stacked plot support for 1D DMFit spectra and warning system for unrecognized kwargs
- Fixed
read_nmr()to accept single string tags - Updated documentation, README, CHANGELOG, and added CITATION.cff
Reviewed changes
Copilot reviewed 14 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Fixed regex pattern by escaping special characters |
| tests/test_spin.py | Updated tests for new grid functionality and SpinCollection changes |
| tests/test_plots.py | Added tests for new dmfit grid functions and kwarg validation |
| tests/test_io.py | Added tests for single-string tag support |
| src/spinplots/spin.py | Added SpinCollection convenience properties and updated plotting logic |
| src/spinplots/plot.py | Major refactoring: added grid functions, unified parameters, extracted helpers |
| src/spinplots/io.py | Fixed tag handling to accept single strings |
| src/spinplots/_helpers.py | New module with shared helper functions |
| src/spinplots/init.py | Updated version and exports |
| pyproject.toml | Version bump to 0.2.2 |
| docs/user_guide/read_export.ipynb | Fixed typo (31C → 13C) and updated examples |
| README.md | Improved quick start guide and feature list |
| CITATION.cff | Added comprehensive citation metadata |
| CHANGELOG.md | Detailed changelog for v0.2.2 |
| .gitignore | Added .claude/ directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1820
to
+1825
| ax.fill_between( | ||
| ppm, | ||
| dmfit_df[f"Line#{j}"], | ||
| alpha=0.3, | ||
| color=deconv_color[j - 1], | ||
| ) |
There was a problem hiding this comment.
In dmfit1d_grid, when deconv_color is a list, the code tries to index it with deconv_color[j - 1] without checking if the list is long enough. If there are more deconvolution lines than colors provided, this will raise an IndexError. Consider adding bounds checking or using modulo indexing like deconv_color[(j - 1) % len(deconv_color)].
Suggested change
| ax.fill_between( | |
| ppm, | |
| dmfit_df[f"Line#{j}"], | |
| alpha=0.3, | |
| color=deconv_color[j - 1], | |
| ) | |
| # Support both a sequence of colors and a single color spec. | |
| if isinstance(deconv_color, (list, tuple)): | |
| if len(deconv_color) > 0: | |
| fill_color = deconv_color[(j - 1) % len(deconv_color)] | |
| else: | |
| fill_color = None | |
| else: | |
| fill_color = deconv_color | |
| if fill_color is not None: | |
| ax.fill_between( | |
| ppm, | |
| dmfit_df[f"Line#{j}"], | |
| alpha=0.3, | |
| color=fill_color, | |
| ) | |
| else: | |
| ax.fill_between( | |
| ppm, | |
| dmfit_df[f"Line#{j}"], | |
| alpha=0.3, | |
| ) |
| spin = read_nmr(DATA_DIR_1D_1, "bruker") | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error") | ||
| _fig, _ax = bruker1d([spin.spectrum], return_fig=True, linewidth=2) |
There was a problem hiding this comment.
Variable _fig is not used.
Suggested change
| _fig, _ax = bruker1d([spin.spectrum], return_fig=True, linewidth=2) | |
| bruker1d([spin.spectrum], return_fig=True, linewidth=2) |
| spin = read_nmr(DATA_DIR_1D_1, "bruker") | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error") | ||
| _fig, _ax = bruker1d([spin.spectrum], return_fig=True, linewidth=2) |
There was a problem hiding this comment.
Variable _ax is not used.
Suggested change
| _fig, _ax = bruker1d([spin.spectrum], return_fig=True, linewidth=2) | |
| bruker1d([spin.spectrum], return_fig=True, linewidth=2) |
| elif colors is not None and cmap is None: | ||
| contour_color = colors[i % len(colors)] | ||
| elif cmap is not None and color is not None: | ||
| raise ValueError("Only one of cmap or color can be provided.") |
There was a problem hiding this comment.
This statement is unreachable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some of the things fixes in this PR, in line with #29 and #31
bruker2d_grid,dmfit1d_grid,dmfit2d_gridwere mising)