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

Transform mask data if orientation does not match volume, fixes #346 #349

Merged
merged 5 commits into from
Dec 11, 2024

Conversation

LukasNickel
Copy link
Contributor

Minimal addition to the code to fix the issue, that the volume and mask data have different orientations.

I avoided ToCanonical and instead only transform the mask data to match the volume.
After trying to implement the transform myself, I decided the safest way is probably to use the nibabel functionality directly. Torchio unfortunately only includes the transformation to canonical coordinates as far as I can tell...

Should be working fine, but I did not really test it much yet, which is why this is marked as draft. Hopefully I have the time for that on monday.

Copy link
Contributor

@github-actions github-actions bot 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 your PR! Please ensure you've read DiffDRR's development guide.

@eigenvivek
Copy link
Owner

Hi @LukasNickel thanks a bunch for the PR. Looks good to me!

To properly merge it, the changes have to be written into notebook (see the dev guide). If that's cumbersome, I can push those changes to this branch sometime soon.

@LukasNickel
Copy link
Contributor Author

LukasNickel commented Dec 9, 2024

Hi @LukasNickel thanks a bunch for the PR. Looks good to me!

To properly merge it, the changes have to be written into notebook (see the dev guide). If that's cumbersome, I can push those changes to this branch sometime soon.

I will have a look, just ran out of time on friday.

Although I have to admit, I initially misunderstood the purpose of the notebooks.
Would have assumed that the API doc is autogenerated from the code and the notebook content matters only for the "Tutorials"

Edit: OH, I had it backwards! You develop in the notebook and export that. That is both pretty neat (although I am undecided on whether its a net positive) and not really clear from the README imo.

@LukasNickel
Copy link
Contributor Author

Turns out I completely skipped over the autogenerated comment in the code.
I was about to write how that might be a good idea to add lol.

There might still be a benefit of making the text in the README clearer, but I would say its 90% on me here and the description is mostly fine.
Just a niche thing to see literate programming in the wild and the autocorrect in my brain did not pick it up, mea culpa!

- Nibabel returns a numpy array, torch expects tensors
@LukasNickel LukasNickel marked this pull request as ready for review December 9, 2024 09:51
@eigenvivek
Copy link
Owner

Hi @LukasNickel thanks for the PR and the feedback. Looks good to me! I updated the README to be clearer.

@eigenvivek eigenvivek mentioned this pull request Dec 11, 2024
@eigenvivek
Copy link
Owner

Closes #346

@eigenvivek eigenvivek merged commit 43c64e5 into eigenvivek:main Dec 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants