Rewrite visualization.py, add no_grad flag on metric

[SunQpark]

Hi, @victoresque This is some additional changes from my previous one. Major changes are adding torch.no_grad() flag for computation of example metrics, and replacing manual wrapping of tensorboard functions with inheritance of __getattr__ method. I also changed string formatting literals used in visualization.py to old style which is compatible with python 3.5 Thanks.

[SunQpark]

Thank you for comments, @borgesa The __getattr__ method will return wrapper function only when the attribute name matches one in the list of tensorboard's method names defined at line 19. I don't think we need to warn users here because,

• someone who tries to call method which is not defined on both SummaryWriter and our WriterTensorboardX will just see AttributeError that the module is not found.
• If name matches one of tensorboard's but attribute is not found(None), this means our writer attribute is None or something else(something about visdom, in the future). We don't have to warn in this cases too, because we already printed warning when visualization class is initialized(line 14~15).

Moreover, doing so will display warnings more than 3 times per step, whenever add_something was called without setting visualization on. I think these methods should stay silent when visualization is not active, so that our users don't want to delete them. For the second bullet,

if attr is None:
    pass
else:
    attr('{}/{}'.format(self.mode, tag), data, self.step, *args, **kwargs)'

we need elsehere. otherwise attr will be called after passing the if statement. I can change this into

if attr is not None:
    attr('{}/{}'.format(self.mode, tag), data, self.step, *args, **kwargs)'

but I prefer the current version since this change will make code simpler but not easier to read. How do you think about this?

[borgesa]

Hi @SunQpark ,

Thank you for the response. I see that I was too fast with my first comments.

First bullet I didn't run this code myself earlier, and I guess I didn't fully understand it. Looked a bit more into it now.

Please check if my current understanding is good:

• Variable 'attr' is only 'None' when 'self.writer' is 'None'
• Therefore, the 'pass' statement inside "WriterTensorboardX.__getattr__" should only run when 'config['visualization']['tensorboardX']' is set to False

Is this correct?

If so, I still have one comment and one question: 1) I believe line 15 should raise the exception. If the user has specified to use 'tensorboardX', I believe it is tidy to break the execution if it is not available (and give the user two options: a: install TensorboardX, b: disable the logger in configuration). 2) I do not understand line 38. Could you explain briefly what it does? The line calls 'object.__getattr__', I guess, but what is the impact of it? When I manipulate a bit to get there, I get the following attribute error: "AttributeError: 'super' object has no attribute '__getattr_\'".

Second bullet point I agree with regards to your answer on the second bullet, the pass is currently required, however, what about the following - third alternative:

def wrapper(tag, data, *args, **kwargs):
    """Returns summary writer method if 'attr' is set."""
    if attr:
        attr('{}/{}'.format(self.mode, tag), data, self.step, *args, **kwargs)'

I find the above quite nice.

[SunQpark]

Yes @borgesa, your understanding about attr is correct.

For line 15, I agree with you that we should do something to make users turn off tensorboard option or install that, but I think stopping the operation is too aggressive for that. Maybe just sending warning message would work fine, so I made it to send UserWarning using the warnings package saying that to install tensorboard or turn off option.

The line 38 is for methods which is not from tensorboardx.SummaryWriter, but from the WriterTensorboardX class itself. From now on, set_step is the only method called by this line but maybe we will have more in future. I added some comment there mentioning what that part is for, and handled the AttributeError to produce more appropriate error message.

For second bullet, my intention for previous code was to make it clear that this function handle does nothing when writer is None, but changed that part as your suggestion and just wrote function docstring to explain that.

Thank you, and let me know if you have more suggestion.