Skip to content

Stop copying whole image before cropping #1308

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

Merged
merged 9 commits into from
Jun 15, 2025

Conversation

StijnvWijn
Copy link
Contributor

Fixes #1305.

Description

A slowdown while sampling patches that are much smaller than the image in Torchio >= 0.20.4

Checklist

  • I have read the CONTRIBUTING docs and have a developer setup ready
  • Changes are
    • Non-breaking (would not break existing functionality)
    • Breaking (would cause existing functionality to change)
  • Tests added or modified to cover the changes
  • In-line docstrings updated
  • Documentation updated
  • This pull request is ready to be reviewed

@fepegar fepegar requested a review from nicoloesch June 4, 2025 21:32
@fepegar
Copy link
Member

fepegar commented Jun 4, 2025

@nicoloesch could you please comment on the general design? I can then fix formatting etc.

Copy link
Contributor

@nicoloesch nicoloesch left a comment

Choose a reason for hiding this comment

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

Really like the changes @StijnvWijn and this is exactly how I would have done it. Some changes are purely stylistic, others require some feedback from @fepegar about formatting and docstring description to make it as clear as possible.
Most of my comments are suggestions instead of requirements.

@StijnvWijn
Copy link
Contributor Author

StijnvWijn commented Jun 5, 2025

So I was building the docs and I encountered an issue I am not sure how to solve:
The Colin27 dataset is a subclass of Subject, but does not allow the same init parameters as the Subject class and has some private attributes that we can't access with .items(). I tried copying using the __dict__ attribute as suggested here, but I can't seem to initialize a Subject without arguments and i need to initialize the Colin27 without arguments, so I am unsure how to generalize the copy behaviour so that every class gets handled correctly.

I fixed it, but now the apply_transform function has some less beautiful code to make a new subject, copy it and all its attributes, it uses the dict and class attributes, but I added some comments, so hopefully it is clear why.

@StijnvWijn
Copy link
Contributor Author

@nicoloesch Do you have any comments on the new copy mechanism?

Copy link
Contributor

@nicoloesch nicoloesch left a comment

Choose a reason for hiding this comment

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

All my comments have been addressed! Thank you @StijnvWijn

@fepegar fepegar changed the title 1305 Patch sampler speedup Stop copying whole image before cropping Jun 15, 2025
@fepegar
Copy link
Member

fepegar commented Jun 15, 2025

@allcontributors please add @nicoloesch for design, maintenance, question, review

Copy link
Contributor

@fepegar

I've put up a pull request to add @nicoloesch! 🎉

@fepegar fepegar merged commit 5f28b56 into TorchIO-project:main Jun 15, 2025
21 of 22 checks passed
@fepegar
Copy link
Member

fepegar commented Jun 15, 2025

@allcontributors please add @StijnvWijn for code

Copy link
Contributor

@fepegar

I've put up a pull request to add @StijnvWijn! 🎉

@fepegar
Copy link
Member

fepegar commented Jun 15, 2025

Thank you both for your contribution!

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.

PatchSampler slowdown after upgrading
3 participants