Comparison of analytical spectra - #270
Open
Nicolass67 wants to merge 6 commits into
Open
Conversation
- Set figure DPI to 200 for improved image quality. - Introduced global x-axis limits based on data range. - Added handling for peak plotting with error management. - Improved marker handling for different plot types.
There was a problem hiding this comment.
Pull request overview
This PR updates the “combined spectrum” rendering paths so that annotations/overlays (peaks, integrations, multiplicity, info box) are drawn only for the active spectrum (jcamp_idx) rather than for all stacked curves, and factors that overlay logic into a reusable NIComposer.plot_overlays() helper.
Changes:
- Extract overlay plotting from
NIComposer.tf_img()intoNIComposer.plot_overlays()and add_ensure_itg_mpy_from_core_tables()to hydrate integration/multiplicity from JCAMP tables as needed. - Update
TransformerModel.tf_combine()andBagItBaseConverter.__combine_images()to apply overlays only to the active spectrum and to use higher DPI output. - Improve combined plot axis handling (global x-limits/orientation) and remove ad-hoc cyclic voltammetry ref-peak plotting from the combined transformer path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
chem_spectra/model/transformer.py |
Combined-image generation now selects an active spectrum and applies overlays only to that curve; adds global x-limit/orientation logic. |
chem_spectra/lib/converter/bagit/base.py |
BagIt combined-image generation now selects an active composer and applies overlays only to that spectrum. |
chem_spectra/lib/composer/ni.py |
Refactors overlay rendering into plot_overlays() and adds lazy loading of integration/multiplicity data from core tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
278
to
284
| plt.rcParams['figure.figsize'] = [16, 9] | ||
| plt.rcParams['figure.dpi'] = 200 | ||
| plt.rcParams['font.size'] = 14 | ||
| plt.rcParams['legend.loc'] = 'upper left' | ||
| curve_idx = self.params.get('jcamp_idx', 0) | ||
|
|
||
| xlabel, ylabel = '', '' |
Comment on lines
393
to
408
| if (len(self.mpys) == 0 and len(self.core.mpy_itg_table) > 0 | ||
| and not self.core.params['integration'].get('edited') | ||
| and ('originStack' not in self.core.params['integration'])): | ||
| tmp_dic_mpy_peaks = {} | ||
| core_mpy_pks_table = self.core.mpy_pks_table[0] | ||
| for peak in core_mpy_pks_table.split('\n'): | ||
| clear_peak = peak.replace('(', '').replace(')', '') | ||
| split_peak = clear_peak.split(',') | ||
| idx_peakStr = split_peak[0].strip() | ||
| if idx_peakStr not in tmp_dic_mpy_peaks: | ||
| tmp_dic_mpy_peaks[idx_peakStr] = [] | ||
| tmp_dic_mpy_peaks[idx_peakStr].append({ | ||
| 'x': float(split_peak[1].strip()), | ||
| 'y': float(split_peak[2].strip()), | ||
| }) | ||
|
|
Resolve conflict in NIComposer: keep the plot_overlays() extraction and fold in master's changes — __draw_integrals no longer takes itg_h, and multiplicity hydration/plotting is NMR-only.
Address review findings on the overlay extraction: - Skip multiplets without matching peaks when hydrating from JCAMP tables: calc_mpy_center divides by the peak count, so they would crash overlay drawing with ZeroDivisionError. - Guard against a missing/empty $OBSERVEDMULTIPLETSPEAKS table and skip malformed/blank table rows instead of raising IndexError. - Restore cyclic voltammetry ref-peak markers in combined images when the CV state arrives only via the extras form field: backfill list_max_min_peaks on the active composer from the parsed cv_state. - Log exceptions from overlay drawing and x-limit handling instead of swallowing them silently. - Coerce jcamp_idx to int in parse_params and at the combined-image read sites so string form params cannot break active-spectrum selection. - Add tests for table hydration edge cases, plot_overlays boundaries, and combined images with an active spectrum index.
Comment on lines
+71
to
+77
| molfile = open(source_dir_molfile, "r") | ||
| # jcamp_idx picks the active spectrum whose overlays are drawn; a string | ||
| # value must be tolerated (form params arrive untyped) | ||
| params = {**params_1h_jcamp, 'jcamp_idx': '1'} | ||
| tranform_model = TransformerModel(None, molfile=molfile, params=params, multiple_files=[file_1, file_2]) | ||
|
|
||
| tf = tranform_model.tf_combine(list_file_names=['a.dx', 'b.dx']) |
Comment on lines
+1
to
+6
| import matplotlib.pyplot as plt | ||
| import pytest | ||
|
|
||
| from chem_spectra.lib.converter.jcamp.base import JcampBaseConverter | ||
| from chem_spectra.lib.converter.jcamp.ni import JcampNIConverter | ||
| from chem_spectra.lib.composer.ni import NIComposer |
Comment on lines
+85
to
+93
| plt.figure() | ||
| try: | ||
| y_boundary_min, y_boundary_max = ni_composer.plot_overlays( | ||
| plt, adjust_xlim=False, | ||
| ) | ||
| finally: | ||
| plt.clf() | ||
| plt.cla() | ||
|
|
- Open the molfile with a context manager in the combined-overlay test so the descriptor is closed deterministically. - Import pyplot after NIComposer (which selects the Agg backend on import) to keep the test module order-independent. - Close the figure created by the plot_overlays test instead of only clearing it, so figures don't accumulate across the suite.
harivyasi
approved these changes
Jul 17, 2026
harivyasi
left a comment
harivyasi
left a comment
Member
There was a problem hiding this comment.
- Merged master into the branch and resolved the conflicts, so the PR is mergeable again.
- Fixed the issues from review: two crashes when spectra files contain incomplete multiplet data, missing peak markers for cyclic voltammetry in combined images, errors that were silently ignored are now logged, and safer handling of the active-spectrum index (covers the Copilot comments). Also added tests for all of this.
- Small test cleanups from Copilot's second review.
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.
Summary
Combined spectrum views (multi-file transform and BagIt) now show annotations for the active spectrum, not just raw curves.
NIComposer.tf_img()into reusableplot_overlays()(peaks, integrations, multiplicity, info box)_ensure_itg_mpy_from_core_tables())jcamp_idx) inTransformerModel.tf_combine()andBagItBaseConverter.__combine_images()