ignite
ignite copied to clipboard
add NoImprovementHandler
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)
- [X] New doc strings: description and/or example code are in RST format
- [X] Documentation is updated (if required)
@Ishan-Kumar2 It looks very good, thanks ! Please go ahead with the tests !
@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 @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
andtest_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.
@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?
@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.