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 halo implementation and tiling artefact #220

Merged
merged 13 commits into from
Apr 13, 2024
Merged

Conversation

qin-yu
Copy link
Collaborator

@qin-yu qin-yu commented Apr 12, 2024

Fix halo implementation and tiling artefact

  1. Fix halo implementation from mirror padding to actual groundtruth
  2. Fix normalisation from patch-level intensity normalisation to image-level
  3. I recommend batch norm for training models for wide applicability

See my fix to pytorch-3dunet wolny/pytorch-3dunet#113 for more details.

image

@qin-yu
Copy link
Collaborator Author

qin-yu commented Apr 12, 2024

I just fixed the problem in workflow:

But it seems that I need to add and modify tests

@lorenzocerrone
Copy link
Collaborator

lorenzocerrone commented Apr 12, 2024

Hey @qin very nice, I like the pydantic validation a lot.. should we also add it to the plantseg configs?
I think it would be so much nicer than the buggy validation I made.

!!!wrong pr!!! This comment was for the ModelZoo PR

This was referenced Apr 12, 2024
@qin-yu qin-yu added bug Something isn't working enhancement New feature or request labels Apr 12, 2024
It is problematic to have two prediction functions doing the same thing. Created an issue #222
@qin-yu qin-yu marked this pull request as ready for review April 12, 2024 13:45
@qin-yu
Copy link
Collaborator Author

qin-yu commented Apr 12, 2024

Hey @lorenzocerrone, I

  • fixed the wrong halo implementation,
  • changed intensity normalisation during inference to be image-level instead of patch-level,
  • fixed the misuse of branch name in conda build workfllow

Please have a look

Copy link
Collaborator

@lorenzocerrone lorenzocerrone left a comment

Choose a reason for hiding this comment

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

Looks good we can merge it :)

@qin-yu
Copy link
Collaborator Author

qin-yu commented Apr 13, 2024

I improved the docstring based on Adrian's feedback: wolny/pytorch-3dunet#113 (comment)

Since I'm already improving the docstrings, I also fixed the arg types and made some simple improvement of code.

I'll merge this and work on the next.

@qin-yu qin-yu merged commit 962ead1 into master Apr 13, 2024
1 check failed
@qin-yu qin-yu deleted the qy/fix-halo-padding branch April 13, 2024 16:00
@qin-yu qin-yu restored the qy/fix-halo-padding branch April 13, 2024 16:00
@qin-yu
Copy link
Collaborator Author

qin-yu commented Apr 13, 2024

Removing the unused argument in get_test_augmentations() introduced a bug. Then I found that I might also have overlooked the previous global normalisation. Going to check it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants