Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better encapsulation of record metadata #371

Open
cx1111 opened this issue May 3, 2022 · 2 comments
Open

Better encapsulation of record metadata #371

cx1111 opened this issue May 3, 2022 · 2 comments

Comments

@cx1111
Copy link
Member

cx1111 commented May 3, 2022

Right now we have the Record class with a huge number of attributes in the top level object. These attributes are varied, fall into many categories, and become very hard to keep track of.

I also find it rather awkward that there is no separate 'header' or 'metadata' type of object. ie. rdheader and rdrecord both return the same type of object.

Initial idea:

  • RecordInfo class for storing all the header data. Includes record and signal specification fields, and comments. rdheader will create this type.
  • Same Record class for WFDB records. The info attribute will be a RecordInfo object.

Having the top level p_signal, d_signal attributes is not my favorite, but I feel like it's rather pointless to have another object to capture these fields.

Open to suggestions.

@tompollard
Copy link
Member

tompollard commented Jun 1, 2022

I agree that some restructuring of the Record class would be helpful:

class Record(BaseRecord, _header.HeaderMixin, _signal.SignalMixin):

RecordInfo class for storing all the header data. Includes record and signal specification fields, and comments. rdheader will create this type.

It would be good to chat about this. I'm getting myself slightly confused about how these classes (e.g. RecordInfo for header information and Record for data) would relate to the files themselves (e.g. x.hea for headers and x.dat for data).

Having the top level p_signal, d_signal attributes is not my favorite, but I feel like it's rather pointless to have another object to capture these fields.

Side note, but it would be nice if these variables had friendlier names. p_signal, d_signal, e_p_signal, and e_d_signal are pretty opaque.

@cx1111
Copy link
Member Author

cx1111 commented Jun 10, 2022

Related discussion in #376 about signals. Agree with providing friendlier signal attributes (or none at all, see the other issue).

The workflow would be demonstrated in the draft PR #388, which I've just left a comment on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants