search

jjhelmus / nmrglue

A module for working with NMR data in Python
BSD 3-Clause "New" or "Revised" License
211 stars 86 forks source link

ENH: Integration of NMR data with units. #45

Closed kfritzsc closed 8 years ago

kfritzsc commented 8 years ago

The nmrglue.integration.integrate and nmrglue.integration.ndintegrate functions should make it easier to integrate data within limits given in arbitrary units. This pull request is in response to Issue #44.

The integrate and ndintegrate fucntions can integrate data between limits given in any units that fileiobase.unit_conversion supports (this was done as suggested by @jjhelmus). In addition, if a noise range is defined the error of integration due to baseline noise is estimated. The integrate function can be used with 1D data, multiple integration ranges can be input to integrate multiple peaks within one spectrum. The ndintegrate function can be used to integrate data of any dimensionality, however currently only one peak may be integrated at a time.

In response to @atomman comment in Issue #44 :

...I would prefer the unit_conv simply to be the ppm scale for the data...

A function to aid in the conversion of a ppm scale to a unit_conversion object was added as fileiobase.uc_from_freqscale. With input of a frequency scale in units of Hz, kHz or ppm and the observation frequency in Mhz uc_from_freqscale will create an unit_conversion object. This function does not work with time domain data arrays.

The addition of uc_from_freqscale also makes it easier to generate a unit_conversion object from simulated testing data.

@jjhelmus I have added tests for each function but I am not sure that I put the tests in the right places. Should the tests go in the main directory folder (tests) or in the module's sub-directory?

jjhelmus commented 8 years ago

This looks great @kfritzsc. Sorry for not getting to reviewing this earlier, I was traveling last week and let GtiHub emails pile up.

The unit tests should go into the modules sub-directory, the tests in the main folder test directory are older tests which make use of the test data not included with nmrglue. A long term goal is to remove these but I have not had the time to write up a good set of unit tests which do not depend on the test data to replace them. I think I saw a style issue or two in the tests, if you wanted to check the file with pep8 when you move it I would appreciate it.

The integration.rst file should be added to the doc/source/reference/index.rst list under the analysis heading so that the new module get included in the documentation.

Lastly it looks like the docstrings need a new additional spaces and odds and ends to properly render with Sphinx. These are extra tricky to get correct, Sphinx and numpydocs are very unforgiving. I'll add inline notes about the changes, if you want to skip these I can fix them before merging.

kfritzsc commented 8 years ago

@jjhelmus no need to apologize, this is an open source project! I appreciate all the time you do put into the project!

I was having some trouble seeing if the changes I made for the sphinx problems fixed the issues. I was rebuilding the docs from scratch ( deleted the build directory and then using make html) but my changes were not reflected in the new html... I will be sure to figure this out before I submit any future code. Could you please fix any remaining sphinx related documentation errors this time?

kfritzsc commented 8 years ago

close #44

jjhelmus commented 8 years ago

Great work @kfritzsc, the Sphinx fixes looked fine. Building the Sphinx docs locally is a bit challenging. I'll be adding a PR shortly that should make this easier.

I added two commits 7f465a6ce and e9225b216a571 before merging to add the analysis/test directory to the package and fix a minor bug. Let me know if either of this were incorrect.