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.

Sign up for GitHub

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

Jump to bottom

Added early stopping feature #1397

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Added early stopping feature #1397

merged 3 commits into from
Dec 11, 2023

Conversation

SkaarFacee
Copy link
Contributor

Added feature request #924

@felixdittrich92 felixdittrich92 added this to the 0.8.0 milestone Dec 4, 2023
@felixdittrich92 felixdittrich92 linked an issue Dec 4, 2023 that may be closed by this pull request
[references][tf/pt] Add early stopping callback #924
Closed
@felixdittrich92 felixdittrich92 self-assigned this Dec 4, 2023
@felixdittrich92 felixdittrich92 added ext: references Related to references folder framework: pytorch Related to PyTorch backend framework: tensorflow Related to TensorFlow backend type: new feature New feature labels Dec 4, 2023
felixdittrich92
felixdittrich92 requested changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

Hey @SkaarFacee 👋

Thanks for working on this :)

The early stopping looks fine on a first view 👍
But could you please revert all the other changes which are not related to your early stopping addition ?

See the div: https://github.com/mindee/doctr/pull/1397/files

references/classification/utils.py Outdated Show resolved Hide resolved
odulcy-mindee
odulcy-mindee requested changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@odulcy-mindee odulcy-mindee left a comment

Choose a reason for hiding this comment

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

Hello @SkaarFacee 👋,

Thank you very much for this contribution !
I also added some comments

references/.DS_Store Outdated Show resolved Hide resolved
references/detection/utils.py Outdated Show resolved Hide resolved
@odulcy-mindee
Copy link
Collaborator

But could you please revert all the other changes which are not related to your early stopping addition ?

@SkaarFacee You just need to rebase your PR on main

@felixdittrich92
Copy link
Contributor

@SkaarFacee @odulcy-mindee For the scope of this PR i think cleaning up the mentioned points is enough wdyt ?
@odulcy-mindee We can discuss afterwards if we want to move it into doctr in another PR are you fine with it ? :)

@odulcy-mindee
Copy link
Collaborator

We can discuss afterwards if we want to move it into doctr in another PR are you fine with it ? :)

@felixdittrich92 Yes, sure !

@SkaarFacee
Copy link
Contributor Author

But could you please revert all the other changes which are not related to your early stopping addition ?

@SkaarFacee You just need to rebase your PR on main

Won't that revert the changes that I made as well ?

@felixdittrich92
Copy link
Contributor

But could you please revert all the other changes which are not related to your early stopping addition ?

@SkaarFacee You just need to rebase your PR on main

Won't that revert the changes that I made as well ?

No rebase keeps your changes but you need to pull the main branch latest changes before

@SkaarFacee SkaarFacee force-pushed the early_stopping_feature branch from 60cb974 to 6c89ef9 Compare December 8, 2023 22:37
@codecov Codecov
Copy link

codecov bot commented Dec 8, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f36c7c9) 95.79% compared to head (22e323c) 95.76%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1397      +/-   ##
==========================================
- Coverage   95.79%   95.76%   -0.03%     
==========================================
  Files         155      155              
  Lines        6942     6942              
==========================================
- Hits         6650     6648       -2     
- Misses        292      294       +2
Flag Coverage Δ
unittests 95.76% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@SkaarFacee
Copy link
Contributor Author

But could you please revert all the other changes which are not related to your early stopping addition ?

@SkaarFacee You just need to rebase your PR on main

Won't that revert the changes that I made as well ?

No rebase keeps your changes but you need to pull the main branch latest changes before

Can you make sure everything is alright now ?

felixdittrich92
felixdittrich92 requested changes Dec 9, 2023
View reviewed changes
Copy link
Contributor

@felixdittrich92 felixdittrich92 left a comment

Choose a reason for hiding this comment

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

Thanks looks mostly fine now :) I have added a few minor things which should be changed in all files :)

references/classification/utils.py Outdated Show resolved Hide resolved
references/classification/utils.py Outdated Show resolved Hide resolved
references/detection/train_pytorch.py Outdated Show resolved Hide resolved
felixT2K
felixT2K approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@felixT2K felixT2K left a comment

Choose a reason for hiding this comment

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

Thanks @SkaarFacee Looks good now 👍🏼

odulcy-mindee
odulcy-mindee approved these changes Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@odulcy-mindee odulcy-mindee left a comment

Choose a reason for hiding this comment

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

Thanks @SkaarFacee ! 👍

@felixdittrich92 felixdittrich92 merged commit 5105f98 into mindee:main Dec 11, 2023
66 of 67 checks passed
@SkaarFacee
Copy link
Contributor Author

Thanks @SkaarFacee ! 👍

Aww thanks. It was really fun. I hope to contribute more :)

@SkaarFacee SkaarFacee deleted the early_stopping_feature branch December 11, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixT2K felixT2K felixT2K approved these changes

@odulcy-mindee odulcy-mindee odulcy-mindee approved these changes

@felixdittrich92 felixdittrich92 felixdittrich92 approved these changes

Assignees

@felixdittrich92 felixdittrich92

Labels
ext: references Related to references folder framework: pytorch Related to PyTorch backend framework: tensorflow Related to TensorFlow backend type: new feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[references][tf/pt] Add early stopping callback
4 participants
@SkaarFacee @odulcy-mindee @felixdittrich92 @felixT2K