search

MIT-LCP / wfdb-python

Native Python WFDB package
MIT License
730 stars 298 forks source link

Add write_dir argument to csv_to_wfdb. Fixes #67. #492

Closed tompollard closed 1 week ago

tompollard commented 2 weeks ago

As discussed in https://github.com/MIT-LCP/wfdb-python/issues/490, https://github.com/MIT-LCP/wfdb-python/blob/34b989e08435c1a82d31bdd2800c4c14147e3e93/wfdb/io/convert/csv.py#L10 currently "strips the path from the input .csv, then writes the output to .dat and .hea".

It's inconvenient not to be able to specify the output directory. This pull request adds a new output_dir argument to the csv_to_wfdb function. By default output_dir is set to None, which will maintain backwards compatibility. Setting output_dir to a directory will mean that output files are saved to this directory.

I have set this to a WIP, because I haven't tested the new behaviour (other than running pytest). @jshaffer94247, if you have an opportunity to test the fix, I'd appreciate your feedback.

tompollard commented 2 weeks ago

Tests are failing on an unrelated issue, presumably related to an update to Numpy (e.g. this? https://numpy.org/devdocs/release/1.24.0-notes.html#conversion-of-out-of-bound-python-integers)

bemoody commented 2 weeks ago

It should be write_dir to be consistent with wrsamp, wrann, etc.

The existing logic is pretty broken, though! split(os.sep) is decidedly wrong for Windows, but also replace(".csv", "") is screwy. You want to take the basename and then remove anything after a dot.

bemoody commented 2 weeks ago

No test cases - please add a test case so we can see that this function works.

tompollard commented 1 week ago

@bemoody this is now ready to review.

bemoody commented 1 week ago

pandas/core/base.py says "tolist is not deprecated" (to_list is an alias for tolist), so can we simply use tolist?

tompollard commented 1 week ago

pandas/core/base.py says "tolist is not deprecated" (to_list is an alias for tolist), so can we simply use tolist?

I considered this but decided to use to_list() because it sounds like it is what the developers would like us to use (I think for consistency with other pandas methods). I think dropping the try/except is fine though...now done.

tompollard commented 1 week ago

Thanks Benjamin