Move conversion functions to their own modules and remove cyclic imports #370
Conversation
|
|
||
| from wfdb.io import _header | ||
| from wfdb.io import _signal | ||
| from wfdb.io import _url | ||
| from wfdb.io import download | ||
| from wfdb.io import annotation |
There was a problem hiding this comment.
This looks like it used to be a circular import, and I don't know how it was working. See how annotation.py imports record.py. This was only here because of the sigavg function which I moved to the more appropriate processing subpackage.
There was a problem hiding this comment.
I don't quite understand how Python accepts/deals with cyclic deps... https://stackoverflow.com/questions/7336802/how-to-avoid-circular-imports-in-python
| @@ -6,17 +6,10 @@ | |||
| rdsamp, | |||
| wrsamp, | |||
| dl_database, | |||
| edf2mit, | |||
There was a problem hiding this comment.
Functions for the extra formats should not be part of the top level module.
cx1111
changed the title
|
Sorry I know this is really hard to review. I thought it'd be too time consuming to get multiple sequential reviews since there is so much stuff to move out of the way. I've manually checked a reasonable amount by:
Unfortunately, a lot of the stuff I've moved didn't have tests. In addition, there were definitely code paths that were flat out incorrect, including unreachable code paths, etc. I don't think it's a huge risk to accept this PR. |
| f.write(struct.pack(">4s", b"data")) | ||
| # The number of bytes in the signal data chunk | ||
| f.write(struct.pack("<I", chunk_bytes)) | ||
| # Write the signal data... the closest I can get to the original implementation |
There was a problem hiding this comment.
Should this kind of comment be more explicit (e.g. added to the docstring?). If there is information loss or differences between python/c implementations, it would be good to make this clear to the user.
There was a problem hiding this comment.
There's a lot that could be improved regarding the code and the docs of these copied over. functions. We can address it over time.
| print("Block align: {}".format(block_align)) | ||
| bits_per_sample = struct.unpack("<H", wave_file.read(2))[0] | ||
| print("Bits per sample: {}".format(bits_per_sample)) | ||
| # I wish this were more precise but unfortunately some information |
There was a problem hiding this comment.
Same comment as above. Should this be noted in the docstring?
The support for reading non-WFDB files is nice to have, but they should be recognized as extras, rather than being part of this library's core purpose. I don't want to have to consider these other formats when developing/updating our core API. In particular, I'm trying to add a streaming API.
In addition, these functions were making the
recordmodule incredibly large. Therefore this PR moves a bunch of peripheral non-WFDB content to their own modules. It also renames some of functions to more appropriately. eg.edf2mit->read_edf(because we're no longer using the "MIT" term and because the function didn't write files anyway)Functionally, this also removes
rdrecord's ability to read EDF and WAV files. Users can use the individual functionsread_wavandread_edfto get WFDB record objects instead.Finally, a lot of content was moved to more appropriate modules to avoid cyclic dependencies.