Bartzi / see

Code for the AAAI 2018 publication "SEE: Towards Semi-Supervised End-to-End Scene Text Recognition"
GNU General Public License v3.0
573 stars 147 forks source link

Dataset enlarging condition fix and different termination #63

Closed janzd closed 5 years ago

janzd commented 5 years ago

I made the fixes that I mentioned in the issue thread.

Sometimes when the dataset gets larger the training gets stuck and I have to kill it. I'm trying to figure out whether the cause is memory related or if it's something in the code.

Bartzi commented 5 years ago

Thx, for our PR!

I do have two questions/suggestions:

  1. The EarlyStopIntervalTrigger is a nice idea. I think it would make more sense to just subclass the Trigger, you used as base and write the __call__ method like this:

    def __call__(self, trainer):
    fire = super().__call__(trainer)
    if self.curriculum.training_finished:
        fire = True
    return fire

    similarly for the __init__ method.

  2. I can not see that you made any changes to the ProgressBar class. Could you point me to those changes?

janzd commented 5 years ago
  1. Yeah, sure. Subclassing definitely makes more sense. I kind of just put it together without realizing that there is nothing preventing from doing that.

  2. The code of ProgressBar in Chainer v3.2 checks whether the stop trigger is of a class IntervalTrigger. https://github.com/chainer/chainer/blob/v3.2.0/chainer/training/extensions/progress_bar.py#L47 I just copypasted ProgressBar from a recent version (v5) where this requirement has already disappeared. In other words, if you run the code in a recent Chainer version, there isn't any need for the added ProgressBar, but if you run it in v3.2 the default ProgressBar will throw an error. That's why I had to add it because the trigger is EarlyStopIntervalTrigger class after my change.

Bartzi commented 5 years ago

So, if you set the superclass of your EarlyStopIntervalTrigger to IntervalTrigger, you'll also solve the problem you have with the ProgressBar. Could you implement this change?

janzd commented 5 years ago

Done. I didn't realize that isinstance() supports class inheritance.

Bartzi commented 5 years ago

Nice, looks very good now =)

Bartzi commented 5 years ago

thank you!