8000 Supporting DateOnly values if VR is DA · Issue #1866 · fo-dicom/fo-dicom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Supporting DateOnly values if VR is DA #1866

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.

Sign up for GitHub

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

Jump to bottom
Open
iberisoft opened this issue Oct 14, 2024 · 13 comments
Open

Supporting DateOnly values if VR is DA #1866

iberisoft opened this issue Oct 14, 2024 · 13 comments
Labels
enhancement

Comments

@iberisoft
Copy link

Describe the bug
DateOnly is a new type to store dates w/o the time part however it is not supported like the DateTime type.

To Reproduce
Call AddOrUpdate for the Patient's Birth Date (0010,0030) tag and a DateOnly value.

Expected behavior
If VR is DA, the AddOrUpdate method must work for both DateTime and DateOnly values.

Environment
Fellow Oak DICOM version: 4.0.8
OS: Windows 10 x64
Platform: .NET 6.0
@iberisoft iberisoft added the new label Oct 14, 2024
@gofal gofal added enhancement and removed new labels Oct 16, 2024
@gofal
Copy link
Contributor
gofal commented Oct 16, 2024

Great enhancement, but this will not be implemented in version 4 any more.
It could be added in version 5. Would you be willing to upgrade to version 5?

@iberisoft
Copy link
Author

Great enhancement, but this will not be implemented in version 4 any more. It could be added in version 5. Would you be willing to upgrade to version 5?

Of course, please go ahead.

@jaime-olivares
Copy link
Contributor

@gofal
I was trying to implement that and then realized that FO-DICOM.Core is compiled for netstardard2.0 only.
It is highly recommendable to add at least net8.0 because there are several performance improvements from 2.0, apart from new data types.

@iberisoft
Copy link
Author

@gofal I was trying to implement that and then realized that FO-DICOM.Core is compiled for netstardard2.0 only. It is highly recommendable to add at least net8.0 because there are several performance improvements from 2.0, apart from new data types.

Thank you, @jaime-olivares! Sorry for my silly question; I am not familiar with the sources. Do you suggest rewriting fo-dicom to support DateOnly on .NET 6+ (please note that DateOnly was introduced more early in .NET 6) but it will be failed/ignored on the other platforms?

@jaime-olivares
Copy link
Contributor

@iberisoft
fo-dicom can be built for multiple frameworks. However, currently it is only built for netstandard2.0
At compile time, we can use a directive to consider DateOnly when it is compiling fo-dicom for .NET 6+
So, if your application is .NET 6+, it will take the corresponding fo-dicom version containing the DateOnly implementation.

@gofal
Copy link
Contributor
gofal commented Jan 21, 2025

did some runtime tests. fo-dicom is just a library calling .net api-methods. So the performance mainly depends on the executing application, that decides, which framework is loaded.
Having a test applicationin net6, hat uses fo-dicom built as netstadard2.0 or that uses fo-dicom built as net6 has pretty much the same performance.
but updating the test application to net8, no matter if then using fo-dicom built as netstandard2, as net6 or even as net8, then really gains performance improvements.

so the golden rule is, to compile at the lowest .net version in which your code compiles, so that you can support as many applications as possible. You only gain the advantages of newer frameworks in libraries, if you acutally are using new classes or features of the new frameworks. This will very likely be in fo-dicom version 6.
In this next version we also can consider to change the api to get or set values to a DicomDataset (see #1922 ).

the issue here is, that there is only one method, and then at runtime there have to be lot of checks to convert the value into a format that fits to the VR of the dicomtag. This is, why only exactly these types can be passed as parameter, which then later are checked in code.
but having typed apis, then it is the compiler that knows at compiletime that DateOnly can implicitly converted to DateTime and then pass this value. the same is then for DateTime? (the nullable variant) or any other custom datatype that has some implicit conversion implemented.

@iberisoft
Copy link
Author

@gofal if I get you right, you would like to compile fo-dicom still under .NET Standard 2.0 but then the DateOnly would not be supported in the tag getting/setting methods where you check value types. Correct?

@jaime-olivares
Copy link
Contributor

I would choose netstandard2.0 (the most compatible) and net8.0 (the most performant) as the two target frameworks. We do the same in other libraries (like here: https://github.com/Efferent-Health/HL7-V2/blob/main/src/HL7-V2.csproj ).
Multi-targeting doesn't hurt existing users and may benefit them in terms of performance, as I expect most of them are using .NET 8+.

@iberisoft
Copy link
Author

@jaime-olivares thanks for explaining. Then you should put the code specific to .NET 8, e.g. working with the DateOnly type, under #if.

I completely agree that now everyone should use .NET 8 at least due to a lack of maintenance of any older version except for .NET Framework.

@jaime-olivares
Copy link
Contributor

yes, I proposed to use an #if.

Apart from the performance improvements in certain classes, JIT compile, and garbage collection, there may be additional benefits of using new classes conditionally, like FrozenDictionary

@gofal
Copy link
Contributor
gofal commented Jan 29, 2025

There already was a discussion about dropping .netstandard2 support and to compile only in .net6/8/9 etc. I expect most benefits of Span, AsyncEnumerables, Generic Math, Pipes, etc.
But when introducing these changes, the the code might get messed up with many #if statements. we already had this in fo-dicom version 4, and it became more and more hard to maintain. This is why version 5 is built for netstandard2 and next version 6 will then support net8 and up.

@gofal
Copy link
Contributor
gofal commented Jan 29, 2025

But for the issue here with DateOnly. Have you seen the method in DicomDataset.DoAdd, where there is one single generic method that takes all objects of any type T and then there are a lot of runtime tests if the type T is assignable from that type or from the other type etc.

Image

Sure we could add here an other line of code for DateOnly (maybe compiler conditional).
But i then we also should add all other types like DateTimeOffset or the nullables DateTime? or DateTimeOffset? or DateOnly? etc.

and this all leads to weaker runtime performance.
This is why I was thinking about another way of api to get or set values. did you take a look at #1922 ? This would be an approach where you can easily use any datadatype for your model and the compiler does the conversion at compiletime in an efficient way. Sadly this approach is not compatible, so I cannot do it in version 5 now.

Or do you consider this issue here with DateOnly as urgent so that just DateOnly should be added to the huge DicomDataset.DoAdd method?

@iberisoft
Copy link
Author

@gofal thanks for sharing your thoughts! Of course, the new approach will implement this issue in the best way.

I do not see it really urgent, our software may convert a DateOnly value to/from DateTime by using the DateOnly.FromDateTime and DateOnly.ToDateTime methods. So this is up to you when and how you are going to support DateOnly and, probably, TimeOnly.

DateTimeOffset seems a too specific type keeping the time zone as well. Getting its DateTime and UtcDateTime properties would be an ambiguous point for fo-dicom so I would skip it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jaime-olivares @gofal @iberisoft
0