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

fix code failure when dicom time fields are empty #755

Closed
wants to merge 4 commits into from

Conversation

jennan
Copy link
Contributor

@jennan jennan commented Apr 29, 2024

Hello,

This pull request is meant to fix an error we are encountering when processing data with empty date/time fields due to anonymization. Please note the fields are not missing but contain an empty string.

I have adapted the code of get_datetime_from_dcm to handle this scenario the same way as if the date/time was missing.

Please let me know if you need more details or changes to this pull request.

Thank you :).

This scenario happens when anonymization empties the field (not removed).
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR.
I wonder if we could figure out some unittest.

Left some recommended changes to make code a little more generic/shorter and thus easier to read. If you do not see a gotcha with the proposed approach, please accept.

Note to myself:

Overall this is inline with the logic we had before -- if either of them is not defined, we do not return.

But when we are talking about anonymization we might then expect the case where date is null-ed out but time is left untouched. But since I do not think it makes sense to choose some random base for the case when only time is known -- we are ok.

heudiconv/dicoms.py Outdated Show resolved Hide resolved
heudiconv/dicoms.py Outdated Show resolved Hide resolved
heudiconv/dicoms.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.04%. Comparing base (424dcdc) to head (0821456).
Report is 1 commits behind head on master.

❗ Current head 0821456 differs from pull request most recent head 7342206. Consider uploading reports for the commit 7342206 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #755   +/-   ##
=======================================
  Coverage   82.04%   82.04%           
=======================================
  Files          42       42           
  Lines        4205     4205           
=======================================
  Hits         3450     3450           
  Misses        755      755           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jennan and others added 3 commits May 2, 2024 09:55
Co-authored-by: Yaroslav Halchenko <[email protected]>
Co-authored-by: Yaroslav Halchenko <[email protected]>
Co-authored-by: Yaroslav Halchenko <[email protected]>
@jennan
Copy link
Contributor Author

jennan commented May 1, 2024

Hi @yarikoptic , sorry for the delay. I have included your changes, but I am equally happy if you merge #756 and close this PR :-).

Copy link

github-actions bot commented May 2, 2024

🚀 Issue was released in v1.1.1 🚀

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 this pull request may close these issues.

2 participants