wfdb-python icon indicating copy to clipboard operation
wfdb-python copied to clipboard

[WIP] Add write_dir argument to csv_to_wfdb. Fixes #67.

Open tompollard opened this issue 1 year ago • 3 comments

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 avatar Jul 01 '24 19:07 tompollard

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)

tompollard avatar Jul 02 '24 15:07 tompollard

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 avatar Jul 02 '24 21:07 bemoody

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

bemoody avatar Jul 03 '24 21:07 bemoody

@bemoody this is now ready to review.

tompollard avatar Jul 09 '24 21:07 tompollard

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

bemoody avatar Jul 11 '24 17:07 bemoody

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 avatar Jul 11 '24 17:07 tompollard

Thanks Benjamin

tompollard avatar Jul 11 '24 18:07 tompollard