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

[Proposal] Usability: Change Event.time => Event.datetime #16

Closed
Miking98 opened this issue Mar 21, 2024 · 6 comments
Closed

[Proposal] Usability: Change Event.time => Event.datetime #16

Miking98 opened this issue Mar 21, 2024 · 6 comments

Comments

@Miking98
Copy link
Collaborator

Miking98 commented Mar 21, 2024

Suggestion to change Event.time => Event.datetime.

  1. time is actually a datetime, so we should call it what it is to avoid confusion
  2. time is confusing -- it could be a datetime (i.e. 3/2/2024 @ 11:49pm) or just the time (i.e. 11:49pm).
  3. Currently, we have Event.time and Measurement.datetime_value. Both are of type datetime, so we should call them the same thing to avoid confusion.

Other suggestion: instead of just Event.datetime, can we put Event.start_datetime and Event.end_datetime into the actual schema for Event (with end_datetime Optional), rather than relegating end to some arbitrary field in the metadata?

  • A lot of labeling functions will depend on having an end_datetime, so it seems a bit confusing to hide this in metadata.
@Miking98 Miking98 changed the title time and datetime_value are confusing Having both time and datetime_value is confusing Mar 21, 2024
@Miking98 Miking98 changed the title Having both time and datetime_value is confusing Feature: Change Event.time => Event.datetime Mar 26, 2024
@Miking98 Miking98 changed the title Feature: Change Event.time => Event.datetime Usability: Change Event.time => Event.datetime Mar 26, 2024
@EthanSteinberg
Copy link
Collaborator

I'd be down to a rename. I don't think it's necessary (and it would make writing code against the meds standard take more time), but if people think it's confusing, Event.datetime is fine.

@jason-fries
Copy link

jason-fries commented Mar 26, 2024

We should decide on the default representational assumption of Event -- is it an interval or a point? There are tradeoffs of course

  1. Events are intervals, requiring Event.start_datetime and Event.end_datetime. When an event is a point, Event.end_datetime is null
  2. Events are points, requiring only Event.start_datetime. When an event is an interval, we materialize as 2 separate point events.

Option 1 places more burden on the ETL side, since end_datetime may require making assumptions on assignment. Option 2 seems more aligned with modern methods of linearizing/tokenization data to discrete sequences.

I guess I favor option 2, since we're learning into representations to feed into a model vs. representations that are more amendable to operations like writing labeling functions or other queries over the data (where interval reasoning might be super useful).

@Miking98
Copy link
Collaborator Author

Miking98 commented Mar 26, 2024 via email

@EthanSteinberg
Copy link
Collaborator

Having separate start and end events would cause issues. I think the best approach is the current one, where we have optional end attributes in the metadata.

@jason-fries
Copy link

I think it depends on how we envision MEDS data being used. Do we actually need to track specific events with a UUID or is it enough to provide an indicator token in the sequence that some end event has occurred? I don't know. It's worth considering what scenarios actually require interval events, but I concede it introduces some complexity at unknown benefit.

Since "tokenization" (i.e., the process of transforming partially ordered clinical events into a sequence) is defined higher in the MEDS transformation stack, I'm fine with using an interval event. However, I sort of hate having to always check metadata to see if an end event is defined, so I'd prefer explicit Event.start_datetime and Event.end_datetime. I don't have a sense for how much this blows up the schema size though.

@EthanSteinberg
Copy link
Collaborator

However, I sort of hate having to always check metadata to see if an end event is defined

You don't need to do that. For 99% of usecases, you only need the start. End events are very special and are application specific. The two main examples I am aware of (visits and prescriptions) have very different semantics and should generally not be handled by the same code anyways.

@EthanSteinberg EthanSteinberg changed the title Usability: Change Event.time => Event.datetime [Proposal] Usability: Change Event.time => Event.datetime Apr 19, 2024
@EthanSteinberg EthanSteinberg added enhancement New feature or request and removed enhancement New feature or request labels Apr 23, 2024
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

3 participants