search

jjhelmus / nmrglue

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

added to_csdm and tests, fixed tecmag udic #152

Closed mccarthy677 closed 2 years ago

mccarthy677 commented 2 years ago

I added a to_csdm function in the converter object. It assumes data has been loaded to the converter object and uses the universal dict in the converter object to build csdm-format data . I also added 1D, 2D, and 3D tests for it (using the bruker and agilent test data). Lastly, I tweaked the tecmag data loading a bit. When I would load in tecmag data, it would give a universal dictionary with four dimensions, but the "obs" and "car" keys would get a list of four values instead of one value per dimension. I fixed this so that it puts one value in each dimension.

kaustubhmote commented 2 years ago

This PR requires updating nmrglue requirements to include csdmpy, which seems like a lot for a single function. Perhaps a better way to do this is to move the import statement inside the to_csdm function so that rest of nmrglue does not need to worry about installing this. You should also have a clear ImportError handling block saying that this function requires csdmpy to be installed and available. Maybe @jjhelmus can comment on whether this will be an acceptable solution.

mccarthy677 commented 2 years ago

Hello again, is there anything else I can do to help this along? Thanks for your help so far!

kaustubhmote commented 2 years ago

@mccarthy677 , I am merging this in now. Sorry for the delay. If there is any issue with this approach, I guess this is a small isolated merge that we can always revisit. Thanks for PR!