Skip to content

Read the JCAMP-DX version as a version, not as the whole record line - #204

Merged
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/jcampdx-version-comment
Aug 10, 2026
Merged

Read the JCAMP-DX version as a version, not as the whole record line#204
headmeister merged 2 commits into
isi-nmr:masterfrom
gdevenyi:fix/jcampdx-version-comment

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #197.

Spec 2.1 makes $$ a JCAMP-DX comment. _detect_version returned the whole line tail and compared it against a list of literals, so a real spnam1 was rejected:

$ head -2 resources/testdata/pv7/full/20210128_122257_LEGO_PHANTOM_API_TEST_1_1/7/spnam1
##TITLE= /d/exp/stan/nmr/lists/wave/bp
##JCAMP-DX= 5.00 $$ BRUKER JCAMP library (alpha version)

JcampdxVersionError: "5.00 $$ BRUKER JCAMP library (alpha version)" is not a valid JCAMP-DX version

Folder.make_tree catches JcampdxVersionError and drops the child, so through a Folder the file just vanished with no diagnostic. The whitelist already carried two library-identification strings rather than versions — the same problem, patched one spelling at a time.

Change

JCAMPDX.parse_version cuts at $$ and takes the first token. _detect_version, the loaded-parameter path in version, and verify_version all go through it, so SUPPORTED_VERSIONS becomes four version numbers.

After the fix both spnam0 and spnam1 in the PV6 and PV7 studies load and report 5.00; acqp still reports 4.24.

Test

test_the_version_record_is_read_as_a_version_not_as_a_whole_line covers all five spellings observed across the corpus, and test_an_unsupported_version_is_still_rejected keeps the rejection path honest. Both build their file inline — no vendor data.

Suite: 2146 passed, 12 skipped.

@gdevenyi
gdevenyi force-pushed the fix/jcampdx-version-comment branch from a77cf0e to 23be73c Compare August 8, 2026 19:50
gdevenyi and others added 2 commits August 8, 2026 16:33
`ruff format --check` reported twelve files as unformatted, so any change
touching one of them dragged unrelated reflow into its diff. Run the
formatter once, on its own, so subsequent changes show only what they
actually change.

Formatting only: no behaviour changes, and `ruff check` is clean before and
after. The suite is unchanged at 2140 passed, 12 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
_detect_version returned everything after ##JCAMP-DX= and checked it against
a list of literal strings. Spec 2.1 makes $$ a JCAMP-DX comment, and
ParaVision writes one on this very record: the spnam1 of the PV6 and PV7
studies carries

    ##JCAMP-DX= 5.00 $$ BRUKER JCAMP library (alpha version)

which was rejected with JcampdxVersionError. Folder catches that and drops
the child, so the shape-pulse definition simply disappeared from the tree
with no diagnostic. The whitelist already carried two library-identification
strings rather than versions -- the same problem, patched one spelling at a
time.

Parse the version instead: cut at $$, then take the first token. The
whitelist becomes four version numbers, and both the loaded-parameter path
and verify_version go through the same parser.

Closes isi-nmr#197

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
@gdevenyi
gdevenyi force-pushed the fix/jcampdx-version-comment branch from 23be73c to 433a6c8 Compare August 8, 2026 20:50
@gdevenyi

gdevenyi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto #213 (ruff format), so this branch carries that commit until #213 merges — merge #213 first and this diff reduces to its own change. Verified after the rebase: ruff check and ruff format --check clean, full suite green.

Integration check across all eleven conformance PRs merged together: 2164 passed, 12 skipped; corpus load test 3202 → 3207 of 3478 with zero newly broken datasets.

@headmeister
headmeister merged commit e53284c into isi-nmr:master Aug 10, 2026
7 checks passed
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.

JCAMP-DX version detection matches the whole ##JCAMP-DX line tail against a whitelist, so an inline $$ comment rejects a real spnam1 (spec 2.1)

2 participants