-
Notifications
You must be signed in to change notification settings - Fork 420
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
feat: Added random noise augmentation to object detection #654
Conversation
1) random horizontal flip 2) random vertical flip
Codecov Report
@@ Coverage Diff @@
## main #654 +/- ##
==========================================
- Coverage 96.00% 95.99% -0.02%
==========================================
Files 129 129
Lines 4807 4818 +11
==========================================
+ Hits 4615 4625 +10
- Misses 192 193 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The modifications seem to be highly similar to #648, is there a reason to have them both? |
|
Alright, we if you open several PRs in parallel, in order to avoid conflict and ease the review, it's better if they are independent. Here you have the same modifications appearing in both PRs 😅 |
Yup, I am making changes in these ones as well :) Thanks |
I am getting a ValueError when I change the batch_size from 8 to any other size.
Could you please take a look:) Thanks a lot! |
That means the dimensions are most likely interpreted in the wrong order 🙃 Traceback below:
? that's the one causing the error but not the traceback :) (traceback = the complete error message + tracing you get when you run your code) Also flake8 isn't happy 😅 |
Mind merging main into this please? #691 has changed a few things 👍 |
Would you mind resolving the conflicts so that we can merge the PR? 🙏 |
Yup, I have resolved the conflicts and have fixed the problem that was causing us to choose arbitrary batch_sizes. Doing some minor code style fixes ta the moment before I push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! As discussed, the PR is too massive to correctly review it. I suggest we do several PRs to progressively integrate:
- uniform photometric transformations (noise, blur, etc.)
- non-uniform photometric transformations (shadows, etc.)
- geometric transformations (flips, rotations, crops, etc.)
Ideally, if we could have those as doctr.transforms
(working on PIL or Torch/TF tensors) that would be perfect! I'll open an issue to track this 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is massive, let's narrow it down to photometric augmentations as suggested previously. For instance, this PR could be only about adding a Noise augmentation. Everything else needs to be taken out, it's way too big for me to perform a careful review
On that note, if we use the dataloader, we do the transformation on pytorch tensor. Considering the operation you are doing in numpy, you can perfectly implement them in PyTorch or TensorFlow. In your case, you want to use it for a PyTorch script, so :
- implement the
RandomGaussianNoise
transformation over here https://github.com/mindee/doctr/blob/main/doctr/transforms/modules/pytorch.py - adds some unittest
- adds the entry in the documentation
- adds an illustration of the augmentation in the PR description
- and finally add the transform in the Compose passed to the dataset 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added some comments!
Also, you'll need to add unittest for this transformation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks!
|
||
Args: | ||
freeze_until (str, optional): last layer to freeze | ||
start_lr (float, optional): initial learning rate | ||
end_lr (float, optional): final learning rate | ||
num_it (int, optional): number of iterations to perform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed on purpose, it's not accurate
With regards to an issue cf. #730 Extends the list of supported data augmentations,
This PR introduces the following modifications:
Your feedback is highly welcomed :)