Add test and docs for fileio.spinsolve

jjhelmus / nmrglue

A module for working with NMR data in Python

BSD 3-Clause "New" or "Revised" License

211 stars 86 forks source link

Add test and docs for fileio.spinsolve #151

Closed dcambie closed 3 years ago

dcambie commented 3 years ago

Few improvements to the spinsolve import module

minor change to acqu.par parsing, e.g. now ["Sample"] == "EtOH" before it was ["Sample"] == "\"EtOH\"" (with additional quote marks)
Added info to docs
Tests added (the spectrum referenced in the test has not been added as the /data/ folder is in .gitignore, where can I put it? Also, how to run the tests for the other modules (e.g. bruker)?

dcambie commented 3 years ago

Ops, sorry I realized only now that the project still supports Python2. By seeing only py3.6 listed in setup.py I assumed it was fine to use pathlib and f-strings. I will revert those changes, sorry for the confusion

dcambie commented 3 years ago

Ops, sorry I realized only now that the project still supports Python2. By seeing only py3.6 listed in setup.py I assumed it was fine to use pathlib and f-strings. I will revert those changes, sorry for the confusion

Should be fine now

kaustubhmote commented 3 years ago

Thanks, @dcambie. I will go through this in a few days and merge this in.

kaustubhmote commented 3 years ago

Sorry, this took so long. All the changes look good to me, except that I don't have the data to test. Can you attach it here? We can put it in later in the repository

dcambie commented 3 years ago

No problem, attached is an example 1D spectrum. 092237-1D EXTENDED+-EtOH.zip

FYI i tried to extend processing to 2D (e.g. COSY) in another branch but I failed. I manage to get a series of FIDs but from then I was stuck on how to transpose to 2D. If someone is interest in that, the commit below adds support to the 2D JCAMPDX dialect used by spinsolve (i.e. in a section called "_datatype_NDNMRFID"): https://github.com/dcambie/nmrglue/commit/ab052c22f4038e60b1feb242c505d78119b2b052

LCageman commented 3 years ago

Thanks for your contributions, @dcambie! I have one suggestion, the function you added, "parse_spinsolve_acqu_line(line)", can also be used for the proc.par file. This file is only generated when using the spinsolve expert software, but the syntax is the same. Therefore I suggest to rename it to "parse_spinsolve_par_line" (or something like that) and use it for both acqu.par and proc.par.

I also attached two spectra, one (carbon spectrum) from the normal spinsolve and one from the expert software. Data Examples.zip

LCageman commented 3 years ago

Don't forget to rename it in the docs as well: doc/source/reference/spinsolve.rst

(I'd do it myself, but can't find any option in the browser to edit it)

dcambie commented 3 years ago

Umh, I used the IDE refactoring tool and enabled lookup in non-code part of the project so that changed also doc/source/reference/spinsolve.rst . I guess that should be ok or did I miss something?

LCageman commented 3 years ago

My bad, you are right. I guess I looked at the wrong place. Have a nice evening!