Decubitus orientation and related changes

dicompyler / dicompyler-core

A library of core radiation therapy modules for DICOM RT used by dicompyler

https://dicompyler-core.readthedocs.io

Other

147 stars 69 forks source link

Decubitus orientation and related changes #285

Closed darcymason closed 2 years ago

darcymason commented 2 years ago

Closes #274.

calculate dose for decubitus orientations
update x,y nomenclature to cols, rows, where appropriate
adapt code to use col LUT and row LUT depending on which direction real-world X and Y run
fix reversed di, dj in GetPatientToPixelLUT
explicitly check for standard image orientation patient (head-first/feet-first, prone/supine/decubitus Lt/decubitus Rt.); raise NotImplementedError otherwise
fix for indexing empty dose grid
tests added for various decubitus known grids

pep8speaks commented 2 years ago

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! :beers:

Comment last updated at 2022-05-18 16:29:00 UTC

bastula commented 2 years ago

@darcymason Thanks for the comprehensive PR. Is this ready for review? Do you have any more commits to add?

darcymason commented 2 years ago

@darcymason Thanks for the comprehensive PR. Is this ready for review? Do you have any more commits to add?

Yes, it should be ready - there are still a couple of style warnings, I wasn't sure which ones need to be strictly adhered to. But the core logic is there, and any remaining style issues can be dealt with after adapting to any review comments.

darcymason commented 2 years ago

Good PR! I think there are a few small discussion points, but if we can resolve/discuss them, it should be good to merge in.

Thanks for the review - I've had a quick look and can address these points, might just take me a few days due to other commitments.

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging 023090ab737f506c7548aedd57ce7cda2fb72408 into 24d9a9bb28a04280a9c4c8d2a0b6e051810eb7cf - view on LGTM.com

new alerts:

1 for Unused local variable

darcymason commented 2 years ago

@bastula, sorry for the delay in addressing this review - I think everything is covered now, please have a look and let me know what you think.

Still a few codacy style issues, for me the lines complained about look standard, and my local style checkers pass (pycodestyle and pydocstyle).

bastula commented 2 years ago

Thanks @darcymason for addressing the changes. I'll try to take a look in the next day or two and see if we can get this merged in.

darcymason commented 2 years ago

Hi @bastula, can I make a friendly nudge on this? I would like this in for a deployment that points to master. Thanks.

bastula commented 2 years ago

@darcymason Sorry I thought I was waiting for more code-level changes to come in. Merged. Thanks for all your hard work on this.

darcymason commented 2 years ago

No problem. Thanks, @bastula.