Update PhysNet.py #197
Update PhysNet.py #197
Conversation
There was a problem hiding this comment.
I'm a bit confused by this change, isn't this exact same standardization done in the trainer file here? What's the point of performing said standardization twice?
If this is necessary, I think some comments should be added somewhere (preferably in the second place that standardization occurs, in the trainer file) explaining why. If it's a mistake to do this twice and the code in the trainer file that already does this is fine as is, I'd recommend not making this change.
|
The main issue is that the testing section in trainer.py does not use normalization. Adding this directly to the end of the model can ensure that normalization is not overlooked. |
That makes sense. In that case, maybe @McJackTang can remove the redundant normalization in the training portion of trainer.py such that normalization only takes place at the end of the model. |
|
@yahskapar @McJackTang Is it ready to merge? I think it looks fine to me. |
I think it's fine to merge. @McJackTang, when you have time and prior to merging, can you also consider commenting within the model file why that normalization is there (e.g., to not overlook normalization when testing)? Alternatively, maybe you can consider adding normalization to the test function in the trainer file instead. I'll approve now either way since the aforementioned can be up to you. |
|
@McJackTang, any plans to merge this? Was there something further you wanted to do with this PR before merging? |
Sorry for the late action. I remember that it would help with training. I just need to double-check the details of the performance of this branch. |
No description provided.