ga4gh-beacon / beacon-v2-Models

Models that leverage the Beacon Framework v2
Apache License 2.0
4 stars 7 forks source link

Procedure and Measurement `dateOfProcedure`, `ageAtProcedure`, `observationMoment`, `date` differences due to fields omitted from `TimeElement` #92

Closed julesjacobsen closed 2 years ago

julesjacobsen commented 2 years ago

https://github.com/ga4gh-beacon/beacon-v2-Models/blob/539cb05dada755c7b1a66b2e187b9668fcfa670b/BEACON-V2-Model/common/procedure.json#L24-L33

https://github.com/ga4gh-beacon/beacon-v2-Models/blob/539cb05dada755c7b1a66b2e187b9668fcfa670b/BEACON-V2-Model/common/measurement.json#L19-L32

Quick note on the nomenclature used here - Class::field: type e.g. Procedure::dateOfProcedure: string

The Beacon Procedure::dateOfProcedure: string, Procedure::ageAtProcedure: TimeElement diverge from the Phenopacket Procedure::performed: TimeElement. Similarly Beacon's Measurement::observationMoment: TimeElement and Measurement::date: string differ from the Phenopacket Measurement::timeObserved: TimeElement field.

The intention of the single TimeElement field for both Measurement::timeObserved and Procedure::performed was to be used with a timestamp, yet allow users to use the more fuzzy definitions if they required to protect the subject's identity. With Beacon, combining the Procedure::dateOfProcedure: string and Procedure::ageAtProcedure: TimeElement (where this example given is the age) allows a recipient to work out the date of birth of the subject, which may not be desired.

In the case of Measurement the date and observationMoment seem redundant as they are both covered by use of a TimeElement::timestamp, which is not possible using the current Beacon TimeElement as the timestamp field is missing, when compared to the Phenopacket version. Also TimeElement is lacking an TimeElement::interval: TimeInterval which makes it impossible to express the duration of one of these concepts.

gsfk commented 2 years ago

There are similar issues with collectionMoment in the biosamples default schema: it's not possible to express an age range, even though this is permitted in phenopackets.

Also, it's not clear to me why collectionMoment and observationMoment use different schemas (collectionMoment is just a bare ISO8601 duration string rather than a timeElement).

https://github.com/ga4gh-beacon/beacon-v2-Models/blob/main/BEACON-V2-Model/biosamples/defaultSchema.json#L41-L45

mbaudis commented 2 years ago

Thanks for the notes - we will evaluate but most of it sounds relevant for follow-up!

Just a comment: Beacon does not a priori build on Phenopackets. Different stakeholders have been involved at timepoints partially preceding PXF v2; and there are a number of areas where the envisioned use of the Beacon default model is better be served by less complex representations, or sometimes by just plainly preferable ones - in Phenopackets many decisions have been informed by rather specific use cases. We have always envisioned the use of alternate models (such as PXF) emerging over time (if needed).

However, since Beacon explicitly wants to avoid definition of its own standards wherever there is a GA4GH alternative, and with PXF v2 now ulfilling AFAIK most of the needs of some major domains, we are in this process of porting & unifying schemas (which would have been much easier through a direct reference to a default schema library).

I would appreciate such generally very helpful reviews even being made better by re-phrasing them to concrete code changes / PRs, saving some Issue/PR round tripping ¯\_(ツ)_/¯