fix(lcms): normalize TIC x to minutes and align UV/TIC pane domains - #323
Open
headri wants to merge 9 commits into
Open
fix(lcms): normalize TIC x to minutes and align UV/TIC pane domains#323headri wants to merge 9 commits into
headri wants to merge 9 commits into
Conversation
extractLcmsParams now scales TIC topic x-values to minutes based on the spectrum's xUnit (SECONDS/RETENTION TIME vs MINUTES), matching the normalization already applied to UVVIS data. LineFocus and MultiFocus also seed their initial auto-fit x-domain from the union of both panes' data (via a new siblingSeed) instead of each pane fitting to its own range independently, so the two panes stay aligned before any zoom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
added 5 commits
August 25, 2026 10:43
The extractParams.js normalizer treated any xUnit containing "TIME" as seconds, but "RETENTION TIME" is ambiguous — ChemStation UVVIS fixtures tag already-in-minutes data with it (e.g. lc_ms_jcamp_uvvis_chemstation.js), while others use it for genuine seconds data. Naively converting on tag alone collapses an already-correct TIC range by 60x (reproduced by retagging lc_ms_jcamp_tic_chemstation.js the way its UVVIS sibling is tagged). The existing UVVIS normalizer in chem.js already guards this correctly (explicit-minutes veto + magnitude sanity check on top of the tag). Extracted it into a shared resolveSecToMinScale helper and apply it to the TIC branch too, at the point in extrSpectraMs where the original per-spectrum xUnit and jcamp.info.UNITS are still in scope (extractParams.js never receives them). extractLcmsParams goes back to a pure passthrough. Scaling the raw spectra/features data at parse time (rather than only a derived topic field) also means every other consumer that reads entity.features[i].data[0].x directly — reducer_hplc_ms's hydrate.js, snapRtToAxis, RT markers, m/z scan matching, submitted peaks, exported descriptions — inherits minutes-scaled values automatically. Verified by dispatching a seconds-tagged TIC through the real hplcMsReducer and added a regression test for it.
extractUvvisView() returns null for a render or two while hplcMs.uvvis.wavelengthIdx catches up to a freshly hydrated feature list. componentDidUpdate skipped recomputing uvvisSeed in that case, but still called multiFocus.update() unconditionally with the resulting empty siblingSeed — collapsing MultiFocus back to its own (possibly shorter) range while LineFocus wasn't redrawn at all, i.e. exactly the domain disagreement this fix exists to eliminate. Added ViewerLineRect.pickSiblingSeed, which caches the last non-empty seed per side (lastTicSeed/lastUvvisSeed) and falls back to it instead of an empty array. Exported ViewerLineRect so this is unit-testable directly.
Sync dist/ (yarn compile) with the src/ changes from the last three commits (TIC minute normalization and union x-domain seeding).
- react/no-unused-class-component-methods: pickSiblingSeed now reads/writes this.lastTicSeed/this.lastUvvisSeed by name instead of via this[cacheKey], so eslint can see they're used. Call sites pass a 'tic'/'uvvis' side instead of the raw cache-key string. - react/sort-comp: moved pickSiblingSeed to after handleUvvisRedo. - import/no-named-as-default: hplc_viewer.js default-imports this module as ViewerLineRect, so exporting the class under that same name as a named export was flagged as confusing. Renamed the test-only named export to UnconnectedViewerLineRect.
…ic guard MultiFocus.setDataParams skips any entity with `if (!feature || !topic) return;` before folding it into its own union via convertTopic (offset 0). collectTicXSeed had no equivalent check, so a TIC entity with a topic but no feature would widen LineFocus's side of the union while contributing nothing to MultiFocus's — the two panes disagreeing by construction, which this fix exists to prevent. collectTicXSeed now applies the same guard and reuses convertTopic directly, so its output is provably identical to what MultiFocus would have included for the same entities. Exported it and added regression tests covering the missing-feature, missing-topic, and mixed-entities cases. Params ordered (layoutSt, ticEntities = []) to keep the defaulted parameter last (default-param-last).
headri
force-pushed
the
619-v4-lcms-uv-and-tic-time-axes-disagree-by-a-factor-of-60
branch
from
August 25, 2026 10:48
6dd8a00 to
731f7b4
Compare
added 3 commits
August 25, 2026 13:10
Replaces the siblingSeed mechanism (each D3 focus class independently falling back to the other pane's data inside setConfig) with a single source of truth: compute the union x-domain once from the TIC/UVVIS data and seed it into Redux via a new seedLcmsUnionExtent action, which reuses updateZoom's existing lcmsSyncX mirroring (reducer_ui.js) to put both sweepExtent[0] and sweepExtent[1] on the identical xExtent. This removes siblingSeed (and its constructor fields/create/update params) entirely from LineFocus and MultiFocus, and removes pickSiblingSeed/collectTicXSeed/lastUvvisSeed/lastTicSeed from ViewerLineRect. ViewerLineRect.maybeSeedUnionXExtent seeds whenever both panes are still unset (initial mount, or right after a zoom reset) and skips seeding while either side has no data yet, so a transiently missing UVVIS view can't commit a partial union. Two mechanisms for the same shared-domain invariant was how the panes could silently desync again after a future change to only one of them; now there's one.
… maybeSeedUnionXExtent
computeLcmsUnionXExtent and maybeSeedUnionXExtent were only tested in
isolation (calling them directly) — nothing proved the real lifecycle
methods derive uvvisSeed/ticEntities from props and call
maybeSeedUnionXExtent with them. Deleting either call site in index.js
would leave every existing test green (verified: removing the
componentDidMount call site makes the new test fail as expected, then
restored it).
Added a wiring test suite that invokes the real (unmocked)
componentDidMount/componentDidUpdate on a plain instance, stubbing only
the DOM-drawing helpers (jest.mock('../common/draw')) and the three D3
focus objects, and asserts seedLcmsUnionExtentAct is called with the
actual props-derived union.
…rting computeLcmsUnionXExtent only ever needed the min/max of both sides, but built two arrays and spread+sorted the combined result. Fold each side directly into a running [xL, xU] pair instead — O(n) rather than O(n log n), and no intermediate x/seed arrays. Note: since the S4 refactor (3dea063), maybeSeedUnionXExtent already guards this behind "both panes still unset", so it now runs only during the brief pre-seed window (mount, or right after a reset) rather than on every render — the O(n log n) cost was real but no longer on a hot path. Applying the fold anyway since it's a strict, free improvement.
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
1 participant
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.
extractLcmsParams now scales TIC topic x-values to minutes based on the spectrum's xUnit (SECONDS/RETENTION TIME vs MINUTES), matching the normalization already applied to UVVIS data. LineFocus and MultiFocus also seed their initial auto-fit x-domain from the union of both panes' data (via a new siblingSeed) instead of each pane fitting to its own range independently, so the two panes stay aligned before any zoom.