[WIP] [RFC] Framework for extending Dataset #989
Conversation
- first step - handle items by tag number
|
Hello @darcymason! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-12-10 01:18:44 UTC |
|
I had a quick look, and so far it looks good to me - will make a proper review probably tomorrow (spent too much time with the VOI LUT PR...) |
| @@ -0,0 +1,53 @@ | |||
| # Copyright 2008-2018 pydicom authors. See LICENSE file for details. | |||
| """Test for encaps.py""" | |||
| existing_tags = set(Dataset._handler_classes.keys()) & set(tags) | ||
| if existing_tags: | ||
| warnings.warn( | ||
| "Registration of handler {!r} is overriding" |
There was a problem hiding this comment.
Wouldn't it make sense to allow more than one handler? I'm thinking of handlers for different SOP classes that may handle the same tag differently. Not sure if that is an intended use case - maybe that would need another mechanism that takes the SOP class into account (similar to the supported TransferSyntax in the image data handlers).
Or I'm just misunderstanding the use case here. I had been thinking about these handler classes as a replacement for the proposed SR classes (SRDocumentStorage, WaveformStorage etc) - maybe you could elaborate a bit about the intended use cases?
There was a problem hiding this comment.
Good points ... I had planned to extend this to tag groups, or slices of tag numbers, which I think could handle most special cases. For example, SR code elements seem to be largely group 0040.
The idea of handling SOP classes is interesting, and we could put handlers in for that as well, but I don't know how to extend that to nested datasets within Sequences, which wouldn't have an SOP class data element. Maybe there is some way in analogy to what was done with SpecificCharacterSet, keeping track of parents and using their handlers. Would take some thought to figure that out. But if someone were building something up from scratch, they might start with some SR content items which they then assemble into an overall dataset. Along the way, there would only be the tag numbers to identify what each data element was.
In terms of more than one handler, I'm not sure how that would work - generally speaking only one operation can actually occur. E.g. for 'get' operations, some data element has to come back, and we would have to allow only one handler to actually provide it. Similarly with setting items, it would be very confusing if multiple handlers could be doing different things to the dataset contents.
In the end, I'd like to see something here that can handle some things with relatively clearly defined conditions. There will no doubt be some cases where subclassing would be the only thing flexible enough to handle all cases.
There was a problem hiding this comment.
I'll add that I need to go back and review the classes in this PR again, and see how/if these ideas can fit. I started with something really general, but should check it in the specifics of this SR code.
There was a problem hiding this comment.
In terms of more than one handler, I'm not sure how that would work - generally speaking only one operation can actually occur.
Yes, I thought about calling handlers until some of them is able to handle it. That would imply that the handlers should know if they want to handle the dataset, and also, that there may be conflicts if two handlers want to handle the same data for the same tag or tag range, so that the first would win... which is probably not a good thing, as it would depend on the order of the handlers. It would actually be similar to the pixel data handlers in this aspect. Also, you are right - if you want to extend this to nested sequences, it would be more complicated.
What I had in mind was actually a generalization of the pixel data handler concept for other tags beside PixelData. So, if this is not really a use case, it may just be a bad idea.
I think your idea is an interesting one. I'm not sure yet how to apply it for SR code, but there are other use cases anyway. I can think for example about handling vendor-specific and/or non-standard-conform tags, or a kind of cheap anonymization - actually there are lots of possible use cases for pydicom users now I think about it. I guess I have been thinking more about using it inside pydicom so far...
|
Leading up to v2.0 release, I'm not convinced this is the right way to go, and don't want to put something in place and later break code because a different path is needed. Pushing this to a later version unless there are objections. |
Describe the changes
Created an "extension" concept for Dataset, where a class can be hooked into the Dataset class for specific tags.
Early at this point, only allows the "get" operation to be intercepted for any item by tag number, or existing items by keyword. Could look at handling tag groups or slices, or even defining new methods or properties (that are not dicom dict items).
Looking for feedback on the overall concept before coding extension by setting operations. See the
test_extensioncode for an example of how it is used.One tricky issue was the handler accessing dataset information without getting into an infinite loop. Used the Dataset context manager to 'turn off' handlers when control passed to the registered handler. Could write a separate context manager instead.
Related to recent discussion in #824.
Tasks
[...]/_build/html/index.html)