Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: conditional header generation for NMR JCAMP export #219

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

Nicolass67
Copy link
Contributor

@Nicolass67 Nicolass67 commented Nov 14, 2024

This pull request addresses an issue where certain JCAMP files, like 356_MR260_NaCp_UHV_1H.jcamp.txt, could be opened directly in NMRium but failed to load through the ELN. Specifically, the NMRium button is enabled; however, when clicked, it only displays the placeholder drag-and-drop screen instead of loading the actual data.

Context:

The root cause was identified in the __header_nmr method in chem_spectra/lib/composer/ni.py, which previously attempted to add headers unconditionally. When any data field was missing, this led to inconsistencies and failed parsing by NMRium.

Solution:
This PR modifies the __header_nmr method to conditionally include each header line only if the corresponding data is present. This prevents incomplete or empty header lines that could disrupt the JCAMP file format.

This PR also adds a unit test to validate that no empty headers are generated. This ensures the integrity of the __header_nmr method and guards against regressions in future updates.

Changes:

  • The method now appends header lines only if each value is non-empty.
  • A structured list (header_lines) is created by checking each data field individually.
  • Improved handling of optional header information, ensuring consistent output for each JCAMP file.
  • Added a unit test (test_ni_composer_no_empty_headers) to verify that no empty headers are generated.
  • Included a test file to validate the new behavior with a JCAMP file containing empty headers.

Testing:

  1. Upload the 356_MR260_NaCp_UHV_1H.jcamp file in jcamp format in the ELN.
  2. Save the dataset and ensure the ChemSpectra file is generated correctly.
  3. Confirm the NMRium button loads the data without issues.
  4. Run the new unit test to validate that no empty headers are generated with this command : coverage run -m pytest -k tests/lib/composer/test_ni_composer.py

This fix will ensure that NMRium parse the file correctly when accessed via the ELN.

Issue : data not loaded into nmrium #221

@PiTrem PiTrem merged commit fbaa71f into master Nov 20, 2024
1 check passed
@PiTrem PiTrem changed the title Fix conditional header generation for NMR JCAMP export fix: conditional header generation for NMR JCAMP export Nov 28, 2024
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