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

added unet example #2786

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

anthonytorlucci
Copy link
Contributor

Pull Request Template

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

This is just an example for the community. No issues were created.

Changes

New example added illustrating how to create and train a UNet architecture for image segmentation.

Testing

No changes to the library code itself; no tests added.

Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.69%. Comparing base (b9653f5) to head (76a4a88).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2786      +/-   ##
==========================================
- Coverage   83.64%   81.69%   -1.95%     
==========================================
  Files         826      851      +25     
  Lines      108746   113982    +5236     
==========================================
+ Hits        90956    93122    +2166     
- Misses      17790    20860    +3070     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@laggui laggui left a comment

Choose a reason for hiding this comment

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

This is awesome 😄 thanks for the addition!

I have a couple of comments below

Comment on lines +254 to +256
pub struct BrainTumorDataset {
dataset: InMemDataset<BrainTumorItem>,
}
Copy link
Member

Choose a reason for hiding this comment

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

I was sure that you were going to use the segmentation dataset you added in #2426 😅

Any particular reason why you didn't? Looks like it should be compatible 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few reasons for implementing the dataset in this example, but I plan to test and build more with the ImageFolderDataset in the future.

  1. It was easier to convert PyTorch code using input and output tensors.
  2. I'm still learning rust (15 years python; 3 years c++ in undergrad and grad school). My background is in geophysics, not computer science.
  3. This approach allowed me to dive a little deeper into how datasets are built and how the data loader is working. There are still some traits and advanced rust features to explore and learn here.

Copy link
Member

Choose a reason for hiding this comment

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

All very good reasons! Was mostly curious since you were the one to add support hehe

We can definitely leave it as is and maybe update it in a follow-up PR if you're up for it.

Copy link
Member

@laggui laggui left a comment

Choose a reason for hiding this comment

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

Awesome! By curiosity, do you have any sample predictions? Haven't tried the example locally yet.

Also, just some minor questions/comments. Otherwise implementation LGTM 🙂

@@ -0,0 +1,12 @@
[package]
Copy link
Member

Choose a reason for hiding this comment

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

If you want you can add your username to the authors field since this is basically your contribution 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will.

Unfortunately, I'm struggling to get some good results. The inferred segmentation image is all zeros. I believe it is due to the class imbalance since there are many more pixels that are not tumors than pixels that are. I'm re-reading the original paper and a few other references right now to refactor the code.

Copy link
Member

Choose a reason for hiding this comment

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

I'm re-reading the original paper and a few other references right now to refactor the code.
No problem!

In this case, I'll mark the PR as draft for now. But when the example works as expected you can mark it as ready! Feel free to poke me if you have any questions.

Comment on lines +4 to +7
The training data used in the example can be downloaded from: [kaggle - nikhilroxtomar - brain-tumor-segmentation ](https://www.kaggle.com/datasets/nikhilroxtomar/brain-tumor-segmentation/data).

> [!NOTE]
> Not all images are the same resolution. Most are 512 x 512 pixels.
Copy link
Member

Choose a reason for hiding this comment

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

In the sections below you describe how to resize the images, is this a required step for the data to be used in this example?

Everything else works for 512 x 512 and I didn't download the data yet to see.

If that's the case, I think we should probably provide the script to pre-process the images (instead of having it hidden in the DATA.md file). Even better would be to automatically download the data as we do for other examples but that's not a hard requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove this too. I had to downsample because my laptop (Apple M2 Pro; 32GB RAM) couldn't really handle the full size images.

@laggui laggui marked this pull request as draft February 24, 2025 13:32
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