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

add NoImprovementHandler #2574

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

add NoImprovementHandler #2574

wants to merge 5 commits into from

Conversation

Ishan-Kumar2
Copy link
Contributor

Fixes #2314

Description:
As described in the issue, added a generalized version of EarlyStopping where the stop_function and pass_function can be user-defined.
@sdesrozis let me know if the approach is correct, I'll start working on the tests.

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: handlers Core Handlers module label May 19, 2022
vfdev-5
vfdev-5 reviewed May 19, 2022
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a 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 @Ishan-Kumar2 !
I left few minor comments to address.

ignite/handlers/early_stopping.py
self.trainer = trainer
self.counter = 0
self.best_score = None # type: Optional[float]
self.logger = setup_logger(__name__ + "." + self.__class__.__name__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want also the logger for NoImprovementHandler ?

ignite/handlers/early_stopping.py
"""

_state_dict_all_req_keys = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to save NoImprovementHandler's internal state we have to keep _state_dict_all_req_keys, IMO

@sdesrozis
Copy link
Contributor

@Ishan-Kumar2 It looks very good, thanks ! Please go ahead with the tests !

@Ishan-Kumar2
Copy link
Contributor Author

Ishan-Kumar2 commented May 23, 2022 •
edited

@sdesrozis @vfdev-5 I have added the tests, a lot of them are similar to the EarlyStopping tests and might not be even needed since they both use the NoImprovementHandler functions. For instance, test_state_dict_no_improvement_handler and test_state_dict both use the same function, and having both may be redundant, should I remove one in this case?

Also, the failing test seems unrelated to this PR

@sdesrozis
Copy link
Contributor

@sdesrozis @vfdev-5 I have added the tests, a lot of them are similar to the EarlyStopping tests and might not be even needed since they both use the NoImprovementHandler functions. For instance, test_state_dict_no_improvement_handler and test_state_dict both use the same function, and having both may be redundant, should I remove one in this case?

Also, the failing test seems unrelated to this PR

Yes please, removing redundancy would be great.

Let's see on our side to fix unrelated issues before merging.

Thanks again and sorry for the late answer.

@Ishan-Kumar2
Copy link
Contributor Author

@sdesrozis I was thinking about the redundancies, while it's true that they both call the same function do you think it is still worth having retaining the test_state_dict as it ensures that the EarlyStopping is inheriting it correctly and it works for its set of stop and pass functions, just as a precaution. The same goes for the others. What do you think?

@sdesrozis
Copy link
Contributor

@Ishan-Kumar2 the tests are both useful especially if we consider the evolution of the library. I was thinking about having an unique and generic test code for the both. It would imply introducing an abstraction for the test result. As the tests are quite small and clear, I'm finally not sure about that. So let's keep it simple.

I will review asap. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vfdev-5 vfdev-5 vfdev-5 left review comments

@sdesrozis sdesrozis sdesrozis left review comments

Assignees
No one assigned
Labels
module: handlers Core Handlers module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add no improvement handler (similar to early stopping handler)
3 participants
@Ishan-Kumar2 @sdesrozis @vfdev-5