ignite icon indicating copy to clipboard operation
ignite copied to clipboard

add NoImprovementHandler

Open Ishan-Kumar2 opened this issue 2 years ago • 5 comments

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 avatar May 19 '22 11:05 Ishan-Kumar2

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

sdesrozis avatar May 20 '22 05:05 sdesrozis

@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

Ishan-Kumar2 avatar May 23 '22 06:05 Ishan-Kumar2

@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.

sdesrozis avatar May 25 '22 08:05 sdesrozis

@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 avatar May 27 '22 10:05 Ishan-Kumar2

@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.

sdesrozis avatar May 27 '22 11:05 sdesrozis