Normalizer class added #148
Conversation
There was a problem hiding this comment.
Nate this looks great and good to merge for me! I've added just a couple of comments on whether we should allow to have a normalize without un_normalize or viceversa. Love the extension! Please feel free to address the comments as you prefer, and you can definitely merge without going through me again.
| def un_normalize(self, tensor: np.ndarray) -> np.ndarray: | ||
| """The inverse of normalize if possible. Otherwise identity.""" | ||
| return tensor | ||
|
|
There was a problem hiding this comment.
I love this! The only comment here is that this may encourage us to be a bit lazy and maybe provide Normalizers with just one of the two methods. Are there cases where one might actually want to have just normalize without un_normalize, or the opposite?
There was a problem hiding this comment.
I would have made un_normalize an abstract method also, but zero_mean_std1 is un-invertible, so I had to allow that use case. The un_normalize I think is used almost exclusively for plotting anyway
| def normalize(self, tensor: np.ndarray) -> np.ndarray: | ||
| tensor -= np.mean(tensor) | ||
| tensor /= np.std(tensor) + EPS | ||
| return tensor |
There was a problem hiding this comment.
I wonder whether here we could store the mean and std in self (like done above for Standardize) and hence allow to have an un_normalize.
There was a problem hiding this comment.
I don't think a Normalizer could/should keep track of the mean and std for every tensor it's called on. Then it would have to store a dictionary and get called with a sample id.
There was a problem hiding this comment.
Agreed, that would be an overkill, especially since we do not un_normalize that often. If we really need the un-normalized tensor and no method is available, we can always let the code re-evaluate tensor_from_file without normalization.
No description provided.