uwmadison-chm / bioread

Utilities to work with files from BIOPAC's AcqKnowlege software
MIT License
66 stars 23 forks source link

Reads "Date created" field from event markers #29

Closed pvelasco closed 4 years ago

pvelasco commented 4 years ago

I found that from AcqKnowledge v4.4.0 on, the date a marker is created is included with the marker. With some reversed-engineering, I figured out it is encoded in 'Uknnown3' (note the typo): https://github.com/uwmadison-chm/bioread/blob/4641503e0c97df51551be224ceddcbad6963080f/bioread/headers.py#L791

This PR adds a new property date_created_utc for the class EventMarker, with the date it was created in UTC. With a minor modification it could be modified to return it in the local timezone.

I think this solves #21 (for files created with AcqKnowledge v4.4.0 or later).

njvack commented 4 years ago

Hi again! Thanks for your patience here, life has been more hectic than normal so I haven't gotten a chance to think about this much.

Thank you for finding this — getting some kind of date created out of .acq files is one of the most common requests.

What would you think about adding a method to biopac.Datafile itself that would guess at the created date for the file, if available? Something like:

def created_at_guess(self):
    # return the minimum created_date from the event markers, or None if there are no markers
    # or our markers don't have created_date

I feel like there's probably a better name for this method, but it's not coming to me now.

Also, returning the date in UTC is, I think, completely fine. Folks can convert that to local time themselves if they want.

njvack commented 4 years ago

I don't have access to Acq right now, but... why do we need to subtract the position of the marker out from the date created? If the field in the marker really is the date/time the marker was created, that's the time we're interested in.

I think it's important to also have the Datafile's access method specify that this is a guess -- if someone deletes the standard markers and then later creates new ones during processing (and I bet people do this!) we'll happily return a valid date, but the wrong one.

probably a better method name is earliest_marker_created_at or something.

njvack commented 4 years ago

I'm merging this as-is; I'll look at the other things yet in master

njvack commented 4 years ago

I... I wonder if this data is in the unknown parts of the graph header as well

pvelasco commented 4 years ago

I don't have access to Acq right now, but... why do we need to subtract the position of the marker out from the date created? If the field in the marker really is the date/time the marker was created, that's the time we're interested in.

Well, for the EventMarker, yes, you probably want the date/time the marker was created. However, for the created_at (or created_at_guess) my intent was to find the date/time the recording (the file) was created. For that purpose, supposing that the earliest marker was created automatically during the recording, I'm subtracting the time from the beginning of the recording up to the creation of the marker (delta_time_first_event). If, from what I can tell, there is a marker created automatically by AcqKnowledge at the beginning of the recording (since v4.4.0), delta_time_first_event will be zero.

I think it's important to also have the Datafile's access method specify that this is a guess -- if someone deletes the standard markers and then later creates new ones during processing (and I bet people do this!) we'll happily return a valid date, but the wrong one.

probably a better method name is earliest_marker_created_at or something.

Yes, this is totally possible. I don't have much experience processing BioPac data. I'm just trying to synchronize physio responses to MRI images acquired at the same time. Feel free to change if you think it makes more sense.