Skip to content
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.

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

Problem with (5200,9229) Shared Functional Groups Sequence for X-ray 3d craniofacial image in 0.3.5 #27

Closed
tjol opened this issue Jul 28, 2023 · 8 comments · Fixed by #37
Labels
Milestone

Comments

@tjol
Copy link

tjol commented Jul 28, 2023

Hi,

I'm validating an X-ray 3d craniofacial image DCM with validate_iods, and I don't understand the output of version 0.3.5 (the file passes with 0.3.4):

Module "Multi-frame Functional Groups":
Tag (0018,9504) (X-Ray 3D Frame Type Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9229)
Tag (0028,9145) (Pixel Value Transformation Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9229)
Tag (0028,9110) (Pixel Measures Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9229)
Tag (0020,9071) (Frame Anatomy Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9229)
Tag (0028,9132) (Frame VOI LUT Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9229)
Tag (0020,9116) (Plane Orientation Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9229)
Tag (0020,9111) (Frame Content Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9230)
Tag (0020,9113) (Plane Position Sequence) is unexpected in
  Multi-frame Functional Groups > (5200,9230)

The way I read

all of these are in fact expected in the functional groups sequences.

Is it possible the validate_iods analysis is wrong here?

@mrbean-bremen
Copy link
Member

Thanks for the report! This indeed looks like a regression introduced with the current version. I will have a closer look later today or over the weekend. Meanwhile it probably makes sense for you to continue using the previous version.
Also, to state the obvious (copied from the README):

Disclaimer: No guarantees are given for the correctness of the results. This is alpha-stage software and mostly thought as a proof of concept. Also check the limitations for validate_iods described below.

As this is the second issue in a a short time, I'll probably dedicate more time to the project in the near future, which I had neglected for quite some time under the assumption that this is not really used by anybody.

@mrbean-bremen
Copy link
Member

Just for the record: currently there is no support for handling functional groups (first point in the TODO list - I'm probably going to write issues for the relevant TODOs). The fact that the errors are now shown is an unfortunate side-effect of another fix, and is due to the fact that the mentioned statement:

”For each IOD that includes this Module, a table is defined in which the permitted Functional Group Macros and their usage is specified.

is currently ignored.
Anyway, I'm going to add support for functional groups now, that should actually not that be that difficult (or so it looks to me now...)

@tjol
Copy link
Author

tjol commented Jul 30, 2023

Thanks for the update!

@mrbean-bremen
Copy link
Member

Ok, "not that difficult" turned out to be not quite wrong, but a bit too optimistic...

@darcymason
Copy link
Member

Ok, "not that difficult" turned out to be not quite wrong, but a bit too optimistic...

@mrbean-bremen, let me know if I can be of any help. I've been watching the repo, but I'm not really up to speed on the code yet. But I can offer to do code reviews if you want, which would help me learn a little.

@mrbean-bremen
Copy link
Member

Thanks @darcymason - I think I first have to clean up my own mess here. Getting the needed information turned out be be quite trivial, but I'm struggling a bit with the logic here to check modules inside of sequence items, which is currently not possible. Not rocket science, but I have to do a bit of refactoring, and wrap my head around the existing code in the first place - I haven't touched it for quite some time.
Though I'm never against code reviews and may take you up on your offer :)

@mrbean-bremen mrbean-bremen added this to the 0.4 milestone Aug 6, 2023
mrbean-bremen added a commit to mrbean-bremen/dicom-validator that referenced this issue Aug 12, 2023
- add checks for macros that are not allowed in shared
  or per-frame functional groups
- add check that macros are not present in both shared
  and per-frame functional groups
- closes pydicom#27
mrbean-bremen added a commit to mrbean-bremen/dicom-validator that referenced this issue Aug 13, 2023
- add checks for macros that are not allowed in shared
  or per-frame functional groups
- add check that macros are not present in both shared
  and per-frame functional groups
- closes pydicom#27
mrbean-bremen added a commit that referenced this issue Aug 13, 2023
- add checks for macros that are not allowed in shared
  or per-frame functional groups
- add check that macros are not present in both shared
  and per-frame functional groups
- closes #27
@mrbean-bremen
Copy link
Member

I just released a new version with the fix, please check! Sorry it took me so long, but I also found a couple of other issues I wanted to fix before a new release.

@tjol
Copy link
Author

tjol commented Aug 30, 2023

Hi, sorry it took so long to get back to you (vacation, etc.)

The output looks good with version 0.4.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants