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

patchcommander rework #69

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

JoeySpronck
Copy link
Collaborator

Complete rework of the patch commander, accompanied by a (temporarily) notebook that showcases that it works and how it works. Notebook is actually also a useful tool for setting up your patch config, doesnt support spacing setup yet though.

One remark. In the previous implementation the offset had to be provided in a already scaled form, while the other attributes do not. I remember that this was really had to debug when I was setting up the sampling logic for nnUNet. I think this should be changed such that it is scaled internally in the patch commander.

@coveralls
Copy link

coveralls commented Oct 18, 2024

Pull Request Test Coverage Report for Build 11668319171

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 20 of 22 (90.91%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 72.689%

Changes Missing Coverage Covered Lines Changed/Added Lines %
wholeslidedata/buffer/patchcommander.py 20 22 90.91%
Totals Coverage Status
Change from base Build 11386276311: 0.06%
Covered Lines: 2233
Relevant Lines: 3072

💛 - Coveralls

@JoeySpronck
Copy link
Collaborator Author

@martvanrijthoven had to fix one issue when the first patch was exactly one patch size outside the 0,0 coordinate

Everything works now, please check the notebook if you want to see how it behaves. I have many plots there that show the output grid, also do some stress testing there

and again:
One remark. In the original implementation the offset had to be provided in a already scaled form, while the other attributes do not. I remember that this was really had to debug when I was setting up the sampling logic for nnUNet. I think this should be changed such that it is scaled internally in the patch commander. Do you agree?

@martvanrijthoven
Copy link
Collaborator

Heey Joey,

Thanks for your work. I will try to look at it asap. But I already have a question. Would it make sense to give all arguments already as scaled? The patchsize, for example, is also already scaled.

@JoeySpronck
Copy link
Collaborator Author

JoeySpronck commented Oct 31, 2024

Hey Mart,

I think we need to provide the settings in a way that makes sense from an ease of use perspective. When setting everything up the iterator:

  • You need to provide the patch size that works with your model patch size, this has nothing to do with scaling, its relative to the spacing on which the model is trained.
  • Therefore I think everything should be relative to this as well, such that you do not need to think in multiple spacings as a user
  • Therefore I think that we need to provide everything unscaled, because that is much easier to set up, conceptualize and play around with

I think scaling the offset internally makes most sense and is consistent with the rest. But I dont want to break older code, therefore for now I left it with a comment

@JoeySpronck
Copy link
Collaborator Author

JoeySpronck commented Nov 1, 2024

@martvanrijthoven, altogether nothing changed conceptually, but sampling patches around the edges with center=True or overlap is corrected now. All old code will work exactly the same, but cases where tissue lies right next to the borders of the images are now sampled correctly. If you want you can double check the notebook.

Scaling/not scaling is another thing that can be changed, but right now I left it as it was so it can be integrated directly without any negative impact.

If you agree with the merge ill delete the notebook, and we can integrate the PR.
If you want we can have a brief call about it.

@JoeySpronck JoeySpronck merged commit 1e4c6ca into DIAGNijmegen:main Nov 7, 2024
2 of 3 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.

3 participants