dicom-rs icon indicating copy to clipboard operation
dicom-rs copied to clipboard

DICOM naive datetime retrieval

Open tomhampshire opened this issue 1 year ago • 5 comments

The primitive to_datetime methods all expect a default, fixed time-offset to be passed, which is then applied to the resulting, timezone-aware datetime struct. The DICOM standard allows for datetime VRs to have an optional suffix for offset from Coordinated Universal Time. In this case, it shouldn't be necessary to pass a fixed time-offset to the methods that retrieve datetime data. It would be very helpful if, for a particular datetime primitive, you could retrieve a time-zone aware datetime if it exists, or a naive datetime if not.

tomhampshire avatar Jan 15 '24 18:01 tomhampshire

Thank you for reporting. It is true that having to pass around a time offset to use by default is a bit unergonomic.

We happen to already have a DICOM date-time type that supposedly admits missing components, but there was probably an oversight when the time offset was made mandatory anyway, and this parameter was inherited from the previous revision. I believe that making DicomDateTime accepting a missing time offset is the way to go. This would allow you to retrieve it without requiring a default offset, and they can be turned into naive date and time values all the same.

This would be a good change to make for the next release, as we are preparing for 0.7.0. Let me know if you would be interesting in pursuing this!

Enet4 avatar Jan 16 '24 10:01 Enet4

Hi - thanks for getting back to me! I agree, having DicomDateTime with a structure similar to:

pub struct DicomDateTime {
    date: DicomDate,
    time: Option<DicomTime>,
    offset: Option<FixedOffset>,
}

(i.e. with an optional offset), then being able to access a chrono::NaiveDateTime if offset is None, or a chrono::DateTime if offset is Some. I would be very interested in pursuing this. Accurate times are quite important for us!!

tomhampshire avatar Jan 16 '24 16:01 tomhampshire

I'm going to roll with something like this, for the time being, but it feels a bit... dirty...

use crate::dicom_objects::instance::DicomInstance;
use dicom::core::DataElement;
use dicom::dictionary_std::tags;

use dicom::core::chrono::{prelude::*, DateTime};

const OFFSET_SECONDS: i32 = -1;

enum AnyDateTime {
    TimezoneAware(DateTime<FixedOffset>),
    Naive(NaiveDateTime),
}
fn convert(element: &DataElement) -> Result<AnyDateTime, Box<dyn std::error::Error>> {
    let default_offset: FixedOffset = FixedOffset::east_opt(OFFSET_SECONDS).unwrap();
    let dicom_date_time = element.to_datetime(default_offset.clone())?;

    // Check to see if the default offset has been used. We can assume that there is no
    // timezone information in this case, and we return a naive equivalent.
    // If the offset has changed, we return a timezone aware value.

    if dicom_date_time.offset().eq(&default_offset) {
        Ok(AnyDateTime::Naive(
            dicom_date_time.to_chrono_datetime()?.naive_local(),
        ))
    } else {
        Ok(AnyDateTime::TimezoneAware(
            dicom_date_time.to_chrono_datetime()?,
        ))
    }
}

tomhampshire avatar Jan 16 '24 17:01 tomhampshire

I see ... How about the DicomDateTime would impl: .is_naive()_-> bool or .has_time_zone() -> bool

.to_naive_datetime -> chrono::NaiveDateTime (would always work) .to_datetime -> Option<chrono::DateTime<FixedOffset>>

jmlaka avatar Jan 19 '24 21:01 jmlaka

I'm starting to look for a solution to this.

@tomhampshire In the meantime, your workaround contains one issue:

let dicom_date_time = element.to_datetime(default_offset.clone())?;

....
dicom_date_time.to_chrono_datetime()?.naive_local()

As a DicomDateTime s precision cannot be known, to_chrono_datetime() might fail, if the value stored is not precise. It's an inherent feature of these date / time values. You will get the same result by calling dicom_date_time.exact(). You can check if the value is precise by `dicom_date_time.is_precise() and then proceed to an exact value.

If not precise, you only can retrieve a DateTimeRange, see AsRange #trait.

jmlaka avatar Jan 21 '24 17:01 jmlaka

Thanks - yes, I came across this problem... I have been using: dicom_date_time.earliest()?.naive_local() I'm not too concerned about the precision for my case, so I believe this should be fine.

tomhampshire avatar Jan 25 '24 17:01 tomhampshire