ssokolen / rnmrfit

Robust NMR Lineshape Fitting in the Frequency Domain
3 stars 2 forks source link

bug in read_jcamp #1

Closed andcastillo closed 5 years ago

andcastillo commented 5 years ago

I'm having an error with this code: library(rnmrfit) jcamp = read_jcamp('../data-test/ethylbenzene/h1_0.jdx')

Error in decompress_asdf(by.line) : could not find function "decompress_asdf"

ssokolen commented 5 years ago

Can you please provide me with your jdx file?

I'll double check if all of the jcamp decompression methods were properly included in the package, but I also want to confirm that the code isn't incorrectly parsing something as compressed jcamp data. I don't have any of my own compressed jcamp data so this portion of the package didn't get validated as well as the rest.

ssokolen commented 5 years ago

As you observed, the read_jcamp() function does not handle XYDATA=(X++(Y..Y)) entries -- fitting functions only use read_jcamp() to process acqus files rather than to import data.

How I fix this will depend on the type of data you have and what you are trying to do with it.

1) If your data does not use ASDF compression, I can upload a quick fix within a day or so

2) If your data does use ASDF compression, it might take me a week or two until I have the time to go through and figure out all the DIF/DUP notation and the consistency checks

3) However, if you are trying to use JCAMP data to do a fit, I will still need to modify the NMRData1D class to allow import from a JCAMP data file -- which will also take a bit of time.

If you let me know what sort of data you are dealing with, I can focus my efforts accordingly.

andcastillo commented 5 years ago

@ssokolen,

Thanks for your answer. I actually have couple of hundred nmr spectra stored in JCAMP, most of them compressed using DIF/DUP. There is an implementation made in JavaScript. It could save you some time: https://github.com/cheminfo-js/jcampconverter/blob/master/src/index.js#L733. I would like to fit them all.

ssokolen commented 5 years ago

OK, so dealing with the DIF/DUP data didn't turn out to be that time consuming once I stopped trying to use Rcpp and switched to plain R. So data import won't be particularly fast but it should now work. Feel free to test read_jcamp() from the "jcamp" branch on your own files to see if there are any errors.

However, fitting JCAMP data brings up another issue. This whole package is framed around using both real and imaginary components. Does your data store both? If so, would you be able to attach a sample file? I don't have a single JCAMP example with R/I data. If not, I'll have to see how easy it is to disable the imaginary data fitting.

andcastillo commented 5 years ago

@ssokolen, I've check and most of the data have both, real and imaginary. I'm sending couple of files to your email. Feel free of publishing them as examples.

Nevertheless, I'm sure I have many jcamp that only contain real data and would be great to fit them as well. It is not a priority, but please consider that option soon.

andcastillo commented 5 years ago

@ssokolen, I've tried read_jcamp with one the files that I provided and I get this error:

Error in FUN(X[[i]], ...) : cannot coerce type 'closure' to vector of type 'logical'

  1. FUN(X[[i]], ...)
  2. lapply(NTUPLES[[j]][["PAGES"]], process_jcamp_entry)
  3. eval(substitute(expr), data, enclos = parent.frame())
  4. eval(substitute(expr), data, enclos = parent.frame())
  5. with.default(out[["blocks"]][[i]], { logic <- names(NTUPLES[[j]]) != "PAGES" NTUPLES[[j]][logic] <- lapply(NTUPLES[[j]][logic], process_jcamp_entry, sep = "[ \t],[ \t]") ...
  6. with(out[["blocks"]][[i]], { logic <- names(NTUPLES[[j]]) != "PAGES" NTUPLES[[j]][logic] <- lapply(NTUPLES[[j]][logic], process_jcamp_entry, sep = "[ \t],[ \t]") ... 2.process_jcamp(out, tags = process.tags, entries = process.entries) 1.read_jcamp("h1_46.jdx")
ssokolen commented 5 years ago

Thanks for the data! I should be able to get the parsing fixed pretty quickly now that I have a real file to test. I'll then work on fitting real/imaginary JCAMP data. Fitting real data alone is a little lower on the priority list, but I'll take a look when I can.

andcastillo commented 5 years ago

Thanks. It works