ga4gh-beacon / beacon-v2-Models

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

adjusting treatment and adding required schemas #85

Closed mbaudis closed 2 years ago

mbaudis commented 2 years ago

This goes back to phenopackets alignment discussions and @pnrobinson 's comment in https://github.com/ga4gh-beacon/beacon-v2-Models/issues/83.

The major change here is to move the single dose / treatment schedule to doseIntervals.

The EHR-related case for an additional drugType enum seemed too special (but this could be added).

Tom-Shorter commented 2 years ago

This seems like a good improvement to me and aligns beacon more closely with phenopackets.

As you are adding in more time based components using the phenopackets model should we also look at updating age to the phenopackets versions i.e. ditch the iso8601duration wrapper around the values? AFAIK iso8601duration is only found in age, all other uses are as examples.

@mbaudis has suggested this change in #68

mbaudis commented 2 years ago

@Tom-Shorter the iso8601duration is in principle correct and its use idem to Phenopackets. The difference to timeInterval is that in the examples for age iso8601duration is not anchored... In principle one can encode a time-span-with-start-date in a single anchored ISO duration and wouldn't even need the timeSpan element; but this isn't common & probably not implemented in most tools (?).

The timeInterval example

{ "start": "1967-11-11T07:30:00+01:00", "end": "1967-11-18T12:00:00+01:00" }

... could be written in ISO as calendar anchored interval:

1967-11-11T07:30:00+01:00/1967-11-18T12:00:00+01:00

... or as start anchor + duration:

1967-11-11T07:30:00+01:00/P6DT19H30M

But I understand that it may be more convenient to use the start/end format...

But age itself can best be expressed as ISO duration (one could add the day of birth as ancjhor w/o extra parameter, but again, this is more for wizardry than practical use...).

Tom-Shorter commented 2 years ago

Thanks for the clarification @mbaudis, I was also thinking that phenopackets didn't use the iso8601Duration wrapper for age when in fact it does.

mbaudis commented 2 years ago

Merging w/ several approvals...