Skip to content

feat: expose nmr-correlation package as a new nmr-cli command - #140

Open
MuhammadAbeerAkmal wants to merge 2 commits into
NFDI4Chem:developmentfrom
MuhammadAbeerAkmal:nmr-correlation-command
Open

feat: expose nmr-correlation package as a new nmr-cli command#140
MuhammadAbeerAkmal wants to merge 2 commits into
NFDI4Chem:developmentfrom
MuhammadAbeerAkmal:nmr-correlation-command

Conversation

@MuhammadAbeerAkmal

@MuhammadAbeerAkmal MuhammadAbeerAkmal commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem:
No CLI command currently exposes nmr-correlation's functionality, requested in #66, build correlation data from NMR spectra given a spectra ZIP URL and molecular formula.

What changed:

  • New correlation command in nmr-cli, following the same pattern as peaks-to-nmrium: fetches spectra from a URL, processes them, and calls nmr-correlation's buildCorrelationData(...).
  • Default tolerances (H: 0.02, C: 0.25) confirmed by @vcnainala
  • Reused/extended the existing spectra-fetching logic in prase-spectra.ts (extracted a shared buildWebSource helper to avoid duplicating URL-parsing logic) instead of rewriting it.
  • Guards against invalid CLI input (NaN tolerance overrides fall back to defaults instead of corrupting results), surfaces internal processing logs in the output, and filters out any spectrum that fails to parse instead of passing it through to buildCorrelationData unchecked.

Known limitation:
While testing, I found a pre-existing, unrelated bug #139 that currently makes every spectrum fail a step of the shared processing pipeline, meaning real cross-spectrum correlation links couldn't be verified end-to-end with public test data. The formula-only fallback path (when no spectra process successfully) is fully verified and correct; the "compares/links real signals" path is implemented per nmr-correlation's documented API but untested pending a fix for #139.

@hamed-musallam

Copy link
Copy Markdown
Collaborator

I will review this PR as soon as I can. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants