From 1bf96a936a6c82879c7b1270e8d373062ab084d8 Mon Sep 17 00:00:00 2001 From: MuhammadAbeerAkmal <56149548+MuhammadAbeerAkmal@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:51:54 +0200 Subject: [PATCH 1/5] feat: expose nmr-correlation package as a new nmr-cli command --- app/scripts/nmr-cli/package-lock.json | 166 +++++++----------- app/scripts/nmr-cli/package.json | 1 + app/scripts/nmr-cli/src/correlation.ts | 76 ++++++++ app/scripts/nmr-cli/src/index.ts | 59 ++++++- .../nmr-cli/src/parse/prase-spectra.ts | 13 +- 5 files changed, 206 insertions(+), 109 deletions(-) create mode 100644 app/scripts/nmr-cli/src/correlation.ts diff --git a/app/scripts/nmr-cli/package-lock.json b/app/scripts/nmr-cli/package-lock.json index dd4eebf..d0dfbd6 100644 --- a/app/scripts/nmr-cli/package-lock.json +++ b/app/scripts/nmr-cli/package-lock.json @@ -19,6 +19,7 @@ "lodash.merge": "^4.6.2", "mf-parser": "^3.7.1", "ml-spectra-processing": "^14.22.0", + "nmr-correlation": "^3.0.2", "nmr-processing": "^22.5.2", "openchemlib": "^9.20.0", "playwright": "1.58.2", @@ -133,6 +134,7 @@ "integrity": "sha512-oX8xrhvpiyRCQkG1MFchB09f+cXftgIXb3a7UUa4Y3wpmZPw5tyZGTLWhlESOLq1Rq6oDlc8npVU2/9xiCuXMA==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "undici-types": "~7.18.0" } @@ -165,15 +167,6 @@ "ml-spectra-processing": "^14.33.0" } }, - "node_modules/@zakodium/nmr-types/node_modules/ml-peak-shape-generator": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-5.5.0.tgz", - "integrity": "sha512-DyBq/u5S+/0F49Hm0OWHwWvPPM1aaCZIs/eI7/fUTWfIm86Yu03YXWMQ3874eq33MLZVgM8KguGkfeGDffCLog==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.15.0" - } - }, "node_modules/@zakodium/nmrium-core": { "version": "0.7.30", "resolved": "https://registry.npmjs.org/@zakodium/nmrium-core/-/nmrium-core-0.7.30.tgz", @@ -218,6 +211,45 @@ "zod": "^4.4.3" } }, + "node_modules/@zakodium/nmrium-core/node_modules/ml-matrix-convolution": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/ml-matrix-convolution/-/ml-matrix-convolution-1.0.0.tgz", + "integrity": "sha512-+gS36VTRjBMtUDXlKlAgSs+pZLfOEY0JF/64QRRNq+6KIPCfPlzVO3P41iSEjp1NPwofhLcIJBgKIZyH6ylSVA==", + "license": "MIT", + "dependencies": { + "ml-fft": "1.3.5" + } + }, + "node_modules/@zakodium/nmrium-core/node_modules/ml-matrix-peaks-finder": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/ml-matrix-peaks-finder/-/ml-matrix-peaks-finder-1.0.0.tgz", + "integrity": "sha512-a3SZrvADI6kAbOvICnnNbacC+UB6xPRbgbKxGhFeISUCoe4M3g3RMp9UaUFNFA/rRjWNtW+rGioeOH8UNPZT/Q==", + "license": "MIT", + "dependencies": { + "ml-disjoint-set": "^1.0.0", + "ml-matrix-convolution": "^1.0.0" + } + }, + "node_modules/@zakodium/nmrium-core/node_modules/ml-peak-shape-generator": { + "version": "4.2.0", + "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-4.2.0.tgz", + "integrity": "sha512-BDtR0rhUor5/4J9pJOEMRnD+QQ5v6ohx+o6MfRRg2e2IOTeZfp/uJcy5Y852v5CsNec1GmYMkd5PYrY0245qlQ==", + "license": "MIT", + "dependencies": { + "cheminfo-types": "^1.1.0" + } + }, + "node_modules/@zakodium/nmrium-core/node_modules/nmr-correlation": { + "version": "2.3.5", + "resolved": "https://registry.npmjs.org/nmr-correlation/-/nmr-correlation-2.3.5.tgz", + "integrity": "sha512-WmJXckcF+epK0u2DVv+LBahvtlbFiCbTtXQsYmq07fze6i6XLPHnFcE1q6HKtiYYbePd6ks2PMOmWQEDdU2XbA==", + "license": "MIT", + "dependencies": { + "cheminfo-types": "^1.8.1", + "ml-matrix-peaks-finder": "^1.0.0", + "ml-peak-shape-generator": "^4.1.4" + } + }, "node_modules/@zip.js/zip.js": { "version": "2.8.23", "resolved": "https://registry.npmjs.org/@zip.js/zip.js/-/zip.js-2.8.23.tgz", @@ -984,15 +1016,6 @@ "ml-spectra-processing": "^14.29.0" } }, - "node_modules/ml-gsd/node_modules/ml-peak-shape-generator": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-5.5.0.tgz", - "integrity": "sha512-DyBq/u5S+/0F49Hm0OWHwWvPPM1aaCZIs/eI7/fUTWfIm86Yu03YXWMQ3874eq33MLZVgM8KguGkfeGDffCLog==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.15.0" - } - }, "node_modules/ml-hash-table": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/ml-hash-table/-/ml-hash-table-1.0.0.tgz", @@ -1036,31 +1059,31 @@ } }, "node_modules/ml-matrix-convolution": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/ml-matrix-convolution/-/ml-matrix-convolution-1.0.0.tgz", - "integrity": "sha512-+gS36VTRjBMtUDXlKlAgSs+pZLfOEY0JF/64QRRNq+6KIPCfPlzVO3P41iSEjp1NPwofhLcIJBgKIZyH6ylSVA==", + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/ml-matrix-convolution/-/ml-matrix-convolution-2.0.0.tgz", + "integrity": "sha512-XuEZf4ZTffAz7oDMG4olkh9aZmsIMr343gPTY+ZvnLWTlMiG+TegRbP4fpA1ju7/IK9q8u3TcC2cxf/N3ydtRA==", "license": "MIT", "dependencies": { - "ml-fft": "1.3.5" + "ml-fft": "^1.3.5" } }, "node_modules/ml-matrix-peaks-finder": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/ml-matrix-peaks-finder/-/ml-matrix-peaks-finder-1.0.0.tgz", - "integrity": "sha512-a3SZrvADI6kAbOvICnnNbacC+UB6xPRbgbKxGhFeISUCoe4M3g3RMp9UaUFNFA/rRjWNtW+rGioeOH8UNPZT/Q==", + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/ml-matrix-peaks-finder/-/ml-matrix-peaks-finder-2.0.1.tgz", + "integrity": "sha512-PSMwStdTMtnLAhnKlFuKU0PCeQhSZxyNuKPx/t9qQ2N/00+ZkQRoY/+pGlJSG3V7BG5Rz8xz90tyYDno7FDlhw==", "license": "MIT", "dependencies": { "ml-disjoint-set": "^1.0.0", - "ml-matrix-convolution": "^1.0.0" + "ml-matrix-convolution": "^2.0.0" } }, "node_modules/ml-peak-shape-generator": { - "version": "4.2.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-4.2.0.tgz", - "integrity": "sha512-BDtR0rhUor5/4J9pJOEMRnD+QQ5v6ohx+o6MfRRg2e2IOTeZfp/uJcy5Y852v5CsNec1GmYMkd5PYrY0245qlQ==", + "version": "5.5.0", + "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-5.5.0.tgz", + "integrity": "sha512-DyBq/u5S+/0F49Hm0OWHwWvPPM1aaCZIs/eI7/fUTWfIm86Yu03YXWMQ3874eq33MLZVgM8KguGkfeGDffCLog==", "license": "MIT", "dependencies": { - "cheminfo-types": "^1.1.0" + "cheminfo-types": "^1.15.0" } }, "node_modules/ml-regression-base": { @@ -1164,15 +1187,6 @@ "ml-spectra-processing": "^14.29.0" } }, - "node_modules/ml-spectra-fitting/node_modules/ml-peak-shape-generator": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-5.5.0.tgz", - "integrity": "sha512-DyBq/u5S+/0F49Hm0OWHwWvPPM1aaCZIs/eI7/fUTWfIm86Yu03YXWMQ3874eq33MLZVgM8KguGkfeGDffCLog==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.15.0" - } - }, "node_modules/ml-spectra-processing": { "version": "14.33.0", "resolved": "https://registry.npmjs.org/ml-spectra-processing/-/ml-spectra-processing-14.33.0.tgz", @@ -1218,14 +1232,14 @@ "license": "MIT" }, "node_modules/nmr-correlation": { - "version": "2.3.5", - "resolved": "https://registry.npmjs.org/nmr-correlation/-/nmr-correlation-2.3.5.tgz", - "integrity": "sha512-WmJXckcF+epK0u2DVv+LBahvtlbFiCbTtXQsYmq07fze6i6XLPHnFcE1q6HKtiYYbePd6ks2PMOmWQEDdU2XbA==", + "version": "3.0.2", + "resolved": "https://registry.npmjs.org/nmr-correlation/-/nmr-correlation-3.0.2.tgz", + "integrity": "sha512-0nEUMNIENB+l9C+gERvefWg4grByUVzwVnOl9RGcLNKDnfsvbLDm44ejkMG/Vw2Lp4+i04WJRZThhX/L1UHaqg==", "license": "MIT", "dependencies": { - "cheminfo-types": "^1.8.1", - "ml-matrix-peaks-finder": "^1.0.0", - "ml-peak-shape-generator": "^4.1.4" + "cheminfo-types": "^1.15.0", + "ml-matrix-peaks-finder": "^2.0.0", + "ml-peak-shape-generator": "^5.2.0" } }, "node_modules/nmr-processing": { @@ -1268,54 +1282,6 @@ "spectrum-generator": "^8.2.1" } }, - "node_modules/nmr-processing/node_modules/ml-matrix-convolution": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/ml-matrix-convolution/-/ml-matrix-convolution-2.0.0.tgz", - "integrity": "sha512-XuEZf4ZTffAz7oDMG4olkh9aZmsIMr343gPTY+ZvnLWTlMiG+TegRbP4fpA1ju7/IK9q8u3TcC2cxf/N3ydtRA==", - "license": "MIT", - "dependencies": { - "ml-fft": "^1.3.5" - } - }, - "node_modules/nmr-processing/node_modules/ml-matrix-peaks-finder": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/ml-matrix-peaks-finder/-/ml-matrix-peaks-finder-2.0.1.tgz", - "integrity": "sha512-PSMwStdTMtnLAhnKlFuKU0PCeQhSZxyNuKPx/t9qQ2N/00+ZkQRoY/+pGlJSG3V7BG5Rz8xz90tyYDno7FDlhw==", - "license": "MIT", - "dependencies": { - "ml-disjoint-set": "^1.0.0", - "ml-matrix-convolution": "^2.0.0" - } - }, - "node_modules/nmr-processing/node_modules/ml-peak-shape-generator": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-5.5.0.tgz", - "integrity": "sha512-DyBq/u5S+/0F49Hm0OWHwWvPPM1aaCZIs/eI7/fUTWfIm86Yu03YXWMQ3874eq33MLZVgM8KguGkfeGDffCLog==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.15.0" - } - }, - "node_modules/nmr-processing/node_modules/nmr-correlation": { - "version": "3.0.1", - "resolved": "https://registry.npmjs.org/nmr-correlation/-/nmr-correlation-3.0.1.tgz", - "integrity": "sha512-0iuce3dLBpdcHn0Q/SX3gHvshRCUyN8X6iL6Y97VcK7JS3g8yZSQCIKjft8jsAOUzWP1rX8c1aCxw3BnCjrBEQ==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.8.1", - "ml-matrix-peaks-finder": "^2.0.0", - "ml-peak-shape-generator": "^4.2.0" - } - }, - "node_modules/nmr-processing/node_modules/nmr-correlation/node_modules/ml-peak-shape-generator": { - "version": "4.2.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-4.2.0.tgz", - "integrity": "sha512-BDtR0rhUor5/4J9pJOEMRnD+QQ5v6ohx+o6MfRRg2e2IOTeZfp/uJcy5Y852v5CsNec1GmYMkd5PYrY0245qlQ==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.1.0" - } - }, "node_modules/num-sort": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/num-sort/-/num-sort-2.1.0.tgz", @@ -1341,7 +1307,8 @@ "version": "9.25.0", "resolved": "https://registry.npmjs.org/openchemlib/-/openchemlib-9.25.0.tgz", "integrity": "sha512-FGTaZLJRTGXNC7khx8QvX/EiQBHpH1ncUbT7YXDJhw/Y/aDUKe5WrUIqTguMEtSs3GUHcRUjNUhfkpxulx2UXw==", - "license": "BSD-3-Clause" + "license": "BSD-3-Clause", + "peer": true }, "node_modules/openchemlib-utils": { "version": "8.18.0", @@ -1433,15 +1400,6 @@ "ml-spectra-processing": "^14.28.1" } }, - "node_modules/spectrum-generator/node_modules/ml-peak-shape-generator": { - "version": "5.5.0", - "resolved": "https://registry.npmjs.org/ml-peak-shape-generator/-/ml-peak-shape-generator-5.5.0.tgz", - "integrity": "sha512-DyBq/u5S+/0F49Hm0OWHwWvPPM1aaCZIs/eI7/fUTWfIm86Yu03YXWMQ3874eq33MLZVgM8KguGkfeGDffCLog==", - "license": "MIT", - "dependencies": { - "cheminfo-types": "^1.15.0" - } - }, "node_modules/string-width": { "version": "7.2.0", "resolved": "https://registry.npmjs.org/string-width/-/string-width-7.2.0.tgz", @@ -1530,6 +1488,7 @@ "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", "dev": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -1625,6 +1584,7 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-4.4.3.tgz", "integrity": "sha512-ytENFjIJFl2UwYglde2jchW2Hwm4GJFLDiSXWdTrJQBIN9Fcyp7n4DhxJEiWNAJMV1/BqWfW/kkg71UDcHJyTQ==", "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/app/scripts/nmr-cli/package.json b/app/scripts/nmr-cli/package.json index 0d3bd4d..a306dbc 100644 --- a/app/scripts/nmr-cli/package.json +++ b/app/scripts/nmr-cli/package.json @@ -25,6 +25,7 @@ "lodash.merge": "^4.6.2", "mf-parser": "^3.7.1", "ml-spectra-processing": "^14.22.0", + "nmr-correlation": "^3.0.2", "nmr-processing": "^22.5.2", "openchemlib": "^9.20.0", "playwright": "1.58.2", diff --git a/app/scripts/nmr-cli/src/correlation.ts b/app/scripts/nmr-cli/src/correlation.ts new file mode 100644 index 0000000..add9d6c --- /dev/null +++ b/app/scripts/nmr-cli/src/correlation.ts @@ -0,0 +1,76 @@ +import { buildCorrelationData } from 'nmr-correlation' +import type { Options as CorrelationOptions, Spectra } from 'nmr-correlation' +import { FifoLogger } from 'fifo-logger' +import { + buildWebSource, + core, + parsingOptions, + processSpectra, +} from './parse/prase-spectra' + +// Default tolerances +const DEFAULT_TOLERANCE_H = 0.02 +const DEFAULT_TOLERANCE_C = 0.25 + +export interface CorrelationInput { + url: string + mf: string + toleranceH?: number + toleranceC?: number +} + +function resolveTolerance(value: number | undefined, fallback: number): number { + return value === undefined || Number.isNaN(value) ? fallback : value +} + +export async function generateCorrelationData(input: CorrelationInput) { + const { url, mf, toleranceH, toleranceC } = input + const logger = new FifoLogger() + + const source = buildWebSource(url) + + const { state } = await core.readFromWebSource(source, { + ...parsingOptions, + logger, + }) + + const spectraBeforeProcessing = state.data ? [...state.data.spectra] : [] + + if (state.data) { + processSpectra( + state.data, + { autoProcessing: true, autoDetection: true }, + logger + ) + } + + // processSpectra replaces a spectrum's array slot with a new object only + // when it successfully parses it; on failure it leaves the original raw + // object in place (see its catch block) instead of removing it. Compare + // by reference against the pre-processing snapshot to filter those out, + // so buildCorrelationData never sees a spectrum it can't actually read. + // Note: a pre-existing bug (see https://github.com/NFDI4Chem/nmrkit/issues/139) + // currently makes every spectrum fail this step, so real cross-spectrum correlation links are untested here. + const spectra = (state.data?.spectra ?? []).filter( + (spectrum, index) => spectrum !== spectraBeforeProcessing[index] + ) + + const options: CorrelationOptions = { + mf, + tolerance: { + H: resolveTolerance(toleranceH, DEFAULT_TOLERANCE_H), + C: resolveTolerance(toleranceC, DEFAULT_TOLERANCE_C), + }, + } + + let correlationData + try { + correlationData = buildCorrelationData(spectra as Spectra, options) + } catch (error) { + throw new Error( + `Failed to build correlation data: ${error instanceof Error ? error.message : String(error)}` + ) + } + + return { ...correlationData, logs: logger.getLogs() } +} diff --git a/app/scripts/nmr-cli/src/index.ts b/app/scripts/nmr-cli/src/index.ts index 74c7000..6eeeb7c 100755 --- a/app/scripts/nmr-cli/src/index.ts +++ b/app/scripts/nmr-cli/src/index.ts @@ -4,6 +4,7 @@ import { parseSpectra } from './parse/prase-spectra' import { generateSpectrumFromPublicationString } from './publication-string' import { generateNMRiumFromPeaks } from './peaks-to-nmrium' import type { PeaksToNMRiumInput } from './peaks-to-nmrium' +import { generateCorrelationData } from './correlation' import { hideBin } from 'yargs/helpers' import { parsePredictionCommand } from './prediction' import { readFileSync } from 'fs' @@ -15,8 +16,15 @@ Usage: nmr-cli [options] Commands: parse-spectra Parse a spectra file to NMRium file parse-publication-string resurrect spectrum from the publication string - predict Predict spectrum from Mol + predict Predict spectrum from Mol peaks-to-nmrium Convert a peak list to NMRium object + correlation Build correlation data from NMR spectra fetched from a URL + +Options for 'correlation' command: + -u, --url Spectra ZIP file URL + --mf Molecular formula + --tolerance-h H tolerance override (default: 0.02) + --tolerance-c C tolerance override (default: 0.25) Options for 'parse-spectra' command: -u, --url File URL @@ -226,12 +234,61 @@ const peaksToNMRiumCommand: CommandModule = { }, } +// Define the correlation command +const correlationCommand: CommandModule = { + command: ['correlation', 'corr'], + describe: 'Build correlation data from NMR spectra fetched from a URL', + builder: yargs => { + return yargs.options({ + u: { + alias: 'url', + describe: 'Spectra ZIP file URL', + type: 'string', + demandOption: true, + nargs: 1, + }, + mf: { + describe: 'Molecular formula', + type: 'string', + demandOption: true, + nargs: 1, + }, + 'tolerance-h': { + describe: 'H tolerance override (default: 0.02)', + type: 'number', + }, + 'tolerance-c': { + describe: 'C tolerance override (default: 0.25)', + type: 'number', + }, + }) + }, + handler: async argv => { + try { + const result = await generateCorrelationData({ + url: argv.u as string, + mf: argv.mf as string, + toleranceH: argv['tolerance-h'] as number | undefined, + toleranceC: argv['tolerance-c'] as number | undefined, + }) + console.log(JSON.stringify(result)) + } catch (error) { + console.error( + 'Error:', + error instanceof Error ? error.message : String(error), + ) + process.exit(1) + } + }, +} + yargs(hideBin(process.argv)) .usage(usageMessage) .command(parseFileCommand) .command(parsePublicationCommand) .command(parsePredictionCommand) .command(peaksToNMRiumCommand) + .command(correlationCommand) .showHelpOnFail(true) .help() .parse() diff --git a/app/scripts/nmr-cli/src/parse/prase-spectra.ts b/app/scripts/nmr-cli/src/parse/prase-spectra.ts index 06b521d..acd2dc6 100644 --- a/app/scripts/nmr-cli/src/parse/prase-spectra.ts +++ b/app/scripts/nmr-cli/src/parse/prase-spectra.ts @@ -196,11 +196,9 @@ async function processAndSerialize( outputResult({ nmriumState: { data, version }, images, logs }, o); } -async function loadSpectrumFromURL(options: RequiredKey, logger: FifoLogger) { - const { u: url } = options; - +function buildWebSource(url: string) { const { pathname: relativePath, origin: baseURL } = new URL(url) - const source = { + return { entries: [ { relativePath, @@ -208,7 +206,12 @@ async function loadSpectrumFromURL(options: RequiredKey, l ], baseURL, } +} + +async function loadSpectrumFromURL(options: RequiredKey, logger: FifoLogger) { + const { u: url } = options; + const source = buildWebSource(url) const { state } = await core.readFromWebSource(source, { ...parsingOptions, logger }); @@ -256,4 +259,4 @@ function parseSpectra(argv: yargs.ArgumentsCamelCase -export { loadSpectrumFromFilePath, loadSpectrumFromURL, parseSpectra } +export { loadSpectrumFromFilePath, loadSpectrumFromURL, parseSpectra, processSpectra, parsingOptions, core, buildWebSource } From fc846443671341138e92857a0798afc7a4c41c1d Mon Sep 17 00:00:00 2001 From: MuhammadAbeerAkmal <56149548+MuhammadAbeerAkmal@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:10:18 +0200 Subject: [PATCH 2/5] Address review: support local directory input, filter for FT spectra, add real yargs defaults/aliases --- app/scripts/nmr-cli/src/correlation.ts | 34 +++++++--- app/scripts/nmr-cli/src/index.ts | 67 +++++++++++-------- .../nmr-cli/src/parse/prase-spectra.ts | 14 ++-- 3 files changed, 73 insertions(+), 42 deletions(-) diff --git a/app/scripts/nmr-cli/src/correlation.ts b/app/scripts/nmr-cli/src/correlation.ts index add9d6c..bb78956 100644 --- a/app/scripts/nmr-cli/src/correlation.ts +++ b/app/scripts/nmr-cli/src/correlation.ts @@ -4,16 +4,18 @@ import { FifoLogger } from 'fifo-logger' import { buildWebSource, core, + loadFileCollection, parsingOptions, processSpectra, } from './parse/prase-spectra' -// Default tolerances +// Default tolerances confirmed by vcnainala on issue #66 const DEFAULT_TOLERANCE_H = 0.02 const DEFAULT_TOLERANCE_C = 0.25 export interface CorrelationInput { - url: string + url?: string + dir?: string mf: string toleranceH?: number toleranceC?: number @@ -24,15 +26,22 @@ function resolveTolerance(value: number | undefined, fallback: number): number { } export async function generateCorrelationData(input: CorrelationInput) { - const { url, mf, toleranceH, toleranceC } = input + const { url, dir, mf, toleranceH, toleranceC } = input const logger = new FifoLogger() - const source = buildWebSource(url) - - const { state } = await core.readFromWebSource(source, { - ...parsingOptions, - logger, - }) + const { state } = url + ? await core.readFromWebSource(buildWebSource(url), { + ...parsingOptions, + logger, + }) + : dir + ? await core.read(await loadFileCollection(dir), { + ...parsingOptions, + logger, + }) + : (() => { + throw new Error('Either a spectra URL or a local directory path is required') + })() const spectraBeforeProcessing = state.data ? [...state.data.spectra] : [] @@ -49,10 +58,15 @@ export async function generateCorrelationData(input: CorrelationInput) { // object in place (see its catch block) instead of removing it. Compare // by reference against the pre-processing snapshot to filter those out, // so buildCorrelationData never sees a spectrum it can't actually read. + // Correlation also requires FT (frequency-domain) spectra specifically — + // a spectrum can successfully parse but still fail the FT-processing step + // (that failure is only logged, not removed from the array), so filter on + // info.isFt too, not just whether parsing itself succeeded. // Note: a pre-existing bug (see https://github.com/NFDI4Chem/nmrkit/issues/139) // currently makes every spectrum fail this step, so real cross-spectrum correlation links are untested here. const spectra = (state.data?.spectra ?? []).filter( - (spectrum, index) => spectrum !== spectraBeforeProcessing[index] + (spectrum, index) => + spectrum !== spectraBeforeProcessing[index] && spectrum?.info?.isFt === true ) const options: CorrelationOptions = { diff --git a/app/scripts/nmr-cli/src/index.ts b/app/scripts/nmr-cli/src/index.ts index 215831e..7ca4b70 100755 --- a/app/scripts/nmr-cli/src/index.ts +++ b/app/scripts/nmr-cli/src/index.ts @@ -22,9 +22,10 @@ Commands: Options for 'correlation' command: -u, --url Spectra ZIP file URL + -dir, --dir-path Local directory path --mf Molecular formula - --tolerance-h H tolerance override (default: 0.02) - --tolerance-c C tolerance override (default: 0.25) + --tolerance-h, --th H tolerance override (default: 0.02) + --tolerance-c, --tc C tolerance override (default: 0.25) Options for 'parse-spectra' command: -u, --url File URL @@ -261,36 +262,48 @@ const peaksToNMRiumCommand: CommandModule = { // Define the correlation command const correlationCommand: CommandModule = { command: ['correlation', 'corr'], - describe: 'Build correlation data from NMR spectra fetched from a URL', + describe: 'Build correlation data from NMR spectra fetched from a URL or a local directory', builder: yargs => { - return yargs.options({ - u: { - alias: 'url', - describe: 'Spectra ZIP file URL', - type: 'string', - demandOption: true, - nargs: 1, - }, - mf: { - describe: 'Molecular formula', - type: 'string', - demandOption: true, - nargs: 1, - }, - 'tolerance-h': { - describe: 'H tolerance override (default: 0.02)', - type: 'number', - }, - 'tolerance-c': { - describe: 'C tolerance override (default: 0.25)', - type: 'number', - }, - }) + return yargs + .options({ + u: { + alias: 'url', + describe: 'Spectra ZIP file URL', + type: 'string', + nargs: 1, + }, + dir: { + alias: 'dir-path', + describe: 'Local directory path', + type: 'string', + nargs: 1, + }, + mf: { + describe: 'Molecular formula', + type: 'string', + demandOption: true, + nargs: 1, + }, + 'tolerance-h': { + alias: 'th', + describe: 'H tolerance override', + type: 'number', + default: 0.02, + }, + 'tolerance-c': { + alias: 'tc', + describe: 'C tolerance override', + type: 'number', + default: 0.25, + }, + }) + .conflicts('u', 'dir') }, handler: async argv => { try { const result = await generateCorrelationData({ - url: argv.u as string, + url: argv.u as string | undefined, + dir: argv.dir as string | undefined, mf: argv.mf as string, toleranceH: argv['tolerance-h'] as number | undefined, toleranceC: argv['tolerance-c'] as number | undefined, diff --git a/app/scripts/nmr-cli/src/parse/prase-spectra.ts b/app/scripts/nmr-cli/src/parse/prase-spectra.ts index 27c6971..e496b12 100644 --- a/app/scripts/nmr-cli/src/parse/prase-spectra.ts +++ b/app/scripts/nmr-cli/src/parse/prase-spectra.ts @@ -219,15 +219,19 @@ async function loadSpectrumFromURL(options: RequiredKey, l } -async function loadSpectrumFromFilePath(options: RequiredKey, logger: FifoLogger) { - const { dir: path, include, exclude } = options; - +function loadFileCollection(path: string, include?: string[], exclude?: string[]) { const dirPath = isAbsolute(path) ? path : join(process.cwd(), path) - const fileCollection = await FileCollection.fromPath(dirPath, { + return FileCollection.fromPath(dirPath, { unzip: { zipExtensions: ['zip', 'nmredata'] }, filter: { include, exclude }, }) +} + +async function loadSpectrumFromFilePath(options: RequiredKey, logger: FifoLogger) { + const { dir: path, include, exclude } = options; + + const fileCollection = await loadFileCollection(path, include, exclude) const { state @@ -260,4 +264,4 @@ function parseSpectra(argv: yargs.ArgumentsCamelCase -export { loadSpectrumFromFilePath, loadSpectrumFromURL, parseSpectra, processSpectra, parsingOptions, core, buildWebSource } +export { loadSpectrumFromFilePath, loadSpectrumFromURL, parseSpectra, processSpectra, parsingOptions, core, buildWebSource, loadFileCollection } From e078af862f312b5e8ab6f417a66d330319c0f54d Mon Sep 17 00:00:00 2001 From: MuhammadAbeerAkmal <56149548+MuhammadAbeerAkmal@users.noreply.github.com> Date: Tue, 8 Sep 2026 12:28:08 +0200 Subject: [PATCH 3/5] Address round 2 review: explicit if/else branching, clarify spectrum filter rationale --- app/scripts/nmr-cli/src/correlation.ts | 56 +++++++++++++++----------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/app/scripts/nmr-cli/src/correlation.ts b/app/scripts/nmr-cli/src/correlation.ts index bb78956..c458cb1 100644 --- a/app/scripts/nmr-cli/src/correlation.ts +++ b/app/scripts/nmr-cli/src/correlation.ts @@ -29,19 +29,22 @@ export async function generateCorrelationData(input: CorrelationInput) { const { url, dir, mf, toleranceH, toleranceC } = input const logger = new FifoLogger() - const { state } = url - ? await core.readFromWebSource(buildWebSource(url), { - ...parsingOptions, - logger, - }) - : dir - ? await core.read(await loadFileCollection(dir), { - ...parsingOptions, - logger, - }) - : (() => { - throw new Error('Either a spectra URL or a local directory path is required') - })() + let state + if (url) { + ;({ state } = await core.readFromWebSource(buildWebSource(url), { + ...parsingOptions, + logger, + })) + } else if (dir) { + ;({ state } = await core.read(await loadFileCollection(dir), { + ...parsingOptions, + logger, + })) + } else { + throw new Error( + 'Either a spectra URL or a local directory path is required' + ) + } const spectraBeforeProcessing = state.data ? [...state.data.spectra] : [] @@ -53,20 +56,27 @@ export async function generateCorrelationData(input: CorrelationInput) { ) } - // processSpectra replaces a spectrum's array slot with a new object only - // when it successfully parses it; on failure it leaves the original raw - // object in place (see its catch block) instead of removing it. Compare - // by reference against the pre-processing snapshot to filter those out, - // so buildCorrelationData never sees a spectrum it can't actually read. - // Correlation also requires FT (frequency-domain) spectra specifically — - // a spectrum can successfully parse but still fail the FT-processing step - // (that failure is only logged, not removed from the array), so filter on - // info.isFt too, not just whether parsing itself succeeded. + // Two independent checks, not redundant with each other: + // 1. Reference check: processSpectra replaces a spectrum's array slot with + // a new object only when initiateDatum1D/initiateDatum2D succeeds; on + // failure it leaves the original raw object in place (see its catch + // block) instead of removing it. info.isFt is set at file-load time, + // before this step even runs, so a spectrum whose source data is + // already tagged FT can still fail here and keep isFt: true on its + // broken, incompletely-initialized object, the isFt check alone + // wouldn't catch that case. + // 2. isFt check: correlation requires FT (frequency-domain) spectra + // specifically; a spectrum can successfully initiate but still fail + // the separate FT-processing step (that failure is only logged, not + // removed from the array), leaving it as valid-but-still-FID data. + // Both together ensure buildCorrelationData only ever sees a spectrum + // that both initiated successfully and is genuinely FT-processed. // Note: a pre-existing bug (see https://github.com/NFDI4Chem/nmrkit/issues/139) // currently makes every spectrum fail this step, so real cross-spectrum correlation links are untested here. const spectra = (state.data?.spectra ?? []).filter( (spectrum, index) => - spectrum !== spectraBeforeProcessing[index] && spectrum?.info?.isFt === true + spectrum !== spectraBeforeProcessing[index] && + spectrum?.info?.isFt === true ) const options: CorrelationOptions = { From cc293d14356f6d8bcc2a829ec6064580ceeb40e5 Mon Sep 17 00:00:00 2001 From: MuhammadAbeerAkmal <56149548+MuhammadAbeerAkmal@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:01:43 +0200 Subject: [PATCH 4/5] Address review: filter correlation spectra by ranges/zones instead of reference equality --- app/scripts/nmr-cli/src/correlation.ts | 35 +++++++++----------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/app/scripts/nmr-cli/src/correlation.ts b/app/scripts/nmr-cli/src/correlation.ts index c458cb1..61e9ea0 100644 --- a/app/scripts/nmr-cli/src/correlation.ts +++ b/app/scripts/nmr-cli/src/correlation.ts @@ -8,6 +8,7 @@ import { parsingOptions, processSpectra, } from './parse/prase-spectra' +import { isSpectrum2D } from './parse/data/data2d/isSpectrum2D' // Default tolerances confirmed by vcnainala on issue #66 const DEFAULT_TOLERANCE_H = 0.02 @@ -46,8 +47,6 @@ export async function generateCorrelationData(input: CorrelationInput) { ) } - const spectraBeforeProcessing = state.data ? [...state.data.spectra] : [] - if (state.data) { processSpectra( state.data, @@ -56,28 +55,18 @@ export async function generateCorrelationData(input: CorrelationInput) { ) } - // Two independent checks, not redundant with each other: - // 1. Reference check: processSpectra replaces a spectrum's array slot with - // a new object only when initiateDatum1D/initiateDatum2D succeeds; on - // failure it leaves the original raw object in place (see its catch - // block) instead of removing it. info.isFt is set at file-load time, - // before this step even runs, so a spectrum whose source data is - // already tagged FT can still fail here and keep isFt: true on its - // broken, incompletely-initialized object, the isFt check alone - // wouldn't catch that case. - // 2. isFt check: correlation requires FT (frequency-domain) spectra - // specifically; a spectrum can successfully initiate but still fail - // the separate FT-processing step (that failure is only logged, not - // removed from the array), leaving it as valid-but-still-FID data. - // Both together ensure buildCorrelationData only ever sees a spectrum - // that both initiated successfully and is genuinely FT-processed. + // buildCorrelationData needs detected ranges (1D) or zones (2D) to find + // correlations, so require isFt plus at least one detected range/zone. + // This also excludes spectra that failed to initiate or failed detection, + // since those never get ranges/zones populated either. // Note: a pre-existing bug (see https://github.com/NFDI4Chem/nmrkit/issues/139) - // currently makes every spectrum fail this step, so real cross-spectrum correlation links are untested here. - const spectra = (state.data?.spectra ?? []).filter( - (spectrum, index) => - spectrum !== spectraBeforeProcessing[index] && - spectrum?.info?.isFt === true - ) + // currently makes every spectrum fail initiation, so real cross-spectrum correlation links are untested here. + const spectra = (state.data?.spectra ?? []).filter(spectrum => { + if (spectrum?.info?.isFt !== true) return false + return isSpectrum2D(spectrum) + ? (spectrum.zones?.values?.length ?? 0) > 0 + : (spectrum.ranges?.values?.length ?? 0) > 0 + }) const options: CorrelationOptions = { mf, From 7445ef871447eb7ad70c7bdda40fec88579ca07e Mon Sep 17 00:00:00 2001 From: MuhammadAbeerAkmal <56149548+MuhammadAbeerAkmal@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:23:26 +0200 Subject: [PATCH 5/5] Address review: extract readSpectra and filterSpectra helper functions --- app/scripts/nmr-cli/src/correlation.ts | 80 +++++++++++++++++--------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/app/scripts/nmr-cli/src/correlation.ts b/app/scripts/nmr-cli/src/correlation.ts index 61e9ea0..0c8382b 100644 --- a/app/scripts/nmr-cli/src/correlation.ts +++ b/app/scripts/nmr-cli/src/correlation.ts @@ -1,6 +1,7 @@ import { buildCorrelationData } from 'nmr-correlation' import type { Options as CorrelationOptions, Spectra } from 'nmr-correlation' import { FifoLogger } from 'fifo-logger' +import type { NmriumState, Spectrum } from '@zakodium/nmrium-core' import { buildWebSource, core, @@ -22,31 +23,67 @@ export interface CorrelationInput { toleranceC?: number } -function resolveTolerance(value: number | undefined, fallback: number): number { - return value === undefined || Number.isNaN(value) ? fallback : value +interface ReadSpectraOptions { + url?: string + dir?: string } -export async function generateCorrelationData(input: CorrelationInput) { - const { url, dir, mf, toleranceH, toleranceC } = input - const logger = new FifoLogger() +async function readSpectra( + options: ReadSpectraOptions, + logger: FifoLogger +): Promise> { + const { url, dir } = options - let state if (url) { - ;({ state } = await core.readFromWebSource(buildWebSource(url), { + const { state } = await core.readFromWebSource(buildWebSource(url), { ...parsingOptions, logger, - })) - } else if (dir) { - ;({ state } = await core.read(await loadFileCollection(dir), { + }) + return state + } + + if (dir) { + const { state } = await core.read(await loadFileCollection(dir), { ...parsingOptions, logger, - })) - } else { - throw new Error( - 'Either a spectra URL or a local directory path is required' - ) + }) + return state } + throw new Error('Either a spectra URL or a local directory path is required') +} + +// buildCorrelationData needs detected ranges (1D) or zones (2D) to find +// correlations, so require isFt plus at least one detected range/zone. +// This also excludes spectra that failed to initiate or failed detection, +// since those never get ranges/zones populated either. +// Note: a pre-existing bug (see https://github.com/NFDI4Chem/nmrkit/issues/139) +// currently makes every spectrum fail initiation, so real cross-spectrum correlation links are untested here. +function filterSpectra(spectra: Spectrum[]): Spectrum[] { + return spectra.filter(spectrum => { + const { info } = spectrum + if (info.isFt !== true) return false + + if (isSpectrum2D(spectrum)) { + const { zones } = spectrum + return zones.values.length > 0 + } + + const { ranges } = spectrum + return ranges.values.length > 0 + }) +} + +function resolveTolerance(value: number | undefined, fallback: number): number { + return value === undefined || Number.isNaN(value) ? fallback : value +} + +export async function generateCorrelationData(input: CorrelationInput) { + const { url, dir, mf, toleranceH, toleranceC } = input + const logger = new FifoLogger() + + const state = await readSpectra({ url, dir }, logger) + if (state.data) { processSpectra( state.data, @@ -55,18 +92,7 @@ export async function generateCorrelationData(input: CorrelationInput) { ) } - // buildCorrelationData needs detected ranges (1D) or zones (2D) to find - // correlations, so require isFt plus at least one detected range/zone. - // This also excludes spectra that failed to initiate or failed detection, - // since those never get ranges/zones populated either. - // Note: a pre-existing bug (see https://github.com/NFDI4Chem/nmrkit/issues/139) - // currently makes every spectrum fail initiation, so real cross-spectrum correlation links are untested here. - const spectra = (state.data?.spectra ?? []).filter(spectrum => { - if (spectrum?.info?.isFt !== true) return false - return isSpectrum2D(spectrum) - ? (spectrum.zones?.values?.length ?? 0) > 0 - : (spectrum.ranges?.values?.length ?? 0) > 0 - }) + const spectra = filterSpectra(state.data?.spectra ?? []) const options: CorrelationOptions = { mf,