Add base_datetime property #381
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 6 commits
May 31, 2022 11:45
The base_time and base_date attributes of the BaseRecord class, and the parameters to the BaseRecord/Record/MultiRecord constructors, must be a datetime.time object (or None) and a datetime.date object (or None.) The documentation of all three classes erroneously claimed these attributes were strings.
BaseRecord.__init__ takes a large number of arguments and the order is not especially meaningful or memorable; use keyword arguments instead of positional arguments for clarity.
Previously, if a record contained both a base date and base time, then rdrecord would set the undocumented 'base_datetime' attribute accordingly. This attribute is useful to have in general, so define it for all BaseRecords, Records, and MultiRecords. It is defined as a property that aliases the base_date and base_time attributes. That is to say, if the package or application sets record.base_date and record.base_time, then record.base_datetime is calculated automatically; if the package or application sets record.base_datetime, then record.base_date and record.base_time are calculated automatically. Internally, we continue to store the time and date attributes separately, to accommodate records where the base time is defined but the base date is not. Note that when storing a value in record.base_datetime, it must be a naive datetime object. Records don't have a time zone, and therefore trying to set the base time to an aware datetime object is incorrect.
For convenience and consistency, allow passing base_datetime to the BaseRecord, Record, or MultiRecord constructor. Since this attribute is an alias for base_date and base_time, it is an error for the caller to specify this argument while simultaneously specifying base_date or base_time.
_adjust_datetime is called by _arrange_fields, which is called by rdrecord, in order to calculate the base date and time, given that a subset of the record has been selected (using sampfrom). Previously, this function would calculate base_datetime by combining base_date and base_time; this is now unnecessary because base_datetime is set implicitly when base_date and base_time are set. Likewise, it is unnecessary to set base_time and base_date afterwards, because these are set implicitly when base_datetime is set.
Allow passing base_datetime to wfdb.wrsamp, as an alternative to specifying both base_date and base_time. Note that it is an error to specify base_datetime while also specifying base_date or base_time, and we raise a TypeError in that case.
|
Oh, incidentally it looks like this is required in order for pull #380 to work correctly - I didn't even notice that. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
wfdb.rdrecordcurrently sets an undocumented attributebase_datetime, but only if the record has a base date and a base time.It is frequently convenient to have this attribute available, so:
Always set
base_datetime; set it to None if eitherbase_dateorbase_timeis None.Define it as an aliased property so that setting
base_dateandbase_timeimplicitly setsbase_datetimeand vice versa.For consistency, also accept
base_datetimeas an argument towfdb.Record,wfdb.MultiRecord, andwfdb.wrsamp.Also, fix the documentation that incorrectly claimed
base_dateandbase_timewere strings. They weren't.