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

read nmrPipe data from pathlib.Path object #159

Closed j-brady closed 2 years ago

j-brady commented 2 years ago

Hi @kaustubhmote, I made a very minor change to the nmrglue.pipe.read function so that it deals with pathlib.Path objects by converting them back to strings. Not sure whether this is the ideal solution but I wrote a test for it which is passing. Hope that helps and happy to hear alternative ideas.

kaustubhmote commented 2 years ago

This works perfectly fine for me on Linux, and I agree that this is the simplest solution. Can you confirm that it give the correct path on windows as well? That being said, this does seem to defeat the purpose of having a Path object in the first place. Path objects have a read_bytes() method that might work perfectly here. Maybe another if conditional in the sequence checking whether this method exists, and using it if it does would be better here?

j-brady commented 2 years ago

Hi @kaustubhmote, Yes, I agree that it rather defeats the point of using pathlib.Path if we just convert the path back to a string. I added another elif to explicitly use the pathlib.Path in the np.fromfile call in ng.pipe.read. There does not seem to be much benefit to use the read_bytes method though. Below is a simple test I wrote to compare using the read_bytes method vs just np.fromfile.

p = Path("./data/nmrpipe_3d/ft/test001.ft3")
numpy_method = lambda x: np.fromfile(x)
pathlib_method = lambda x: np.frombuffer(p.read_bytes())
timeit.timeit(lambda: pathlib_method(p), number=100000)
timeit.timeit(lambda: numpy_method(p), number=100000)

The time for pathlib_method was ~45s vs ~40s for numpy_method. Not much in it really, as you have to run a lot of iterations before you start to see a significant difference. The pathlib method is just more code so I might prefer the shorter np.fromfile version. What do you think?

I don't have access to a windows machine but I would expect it to work on windows as I constructed the paths using the overloaded / operator in the tests which should make it platform independent.

kaustubhmote commented 2 years ago

I agree, I think this is the simplest solution. I am merging this in now.