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. |
|
Any update on this? |
|
@McJackTang, were you able to double-check some of those details? No worries if not, maybe we can document this PR more in an issue and leave it open for someone else to integrate this change and double-check performance, if needed. |
|
In my opinion, the key point is about the input format. The original PhysNet uses Raw input and the toolbox uses DiffNormlized input. For those acceptable with DiffNormlized input, this version is fine. For those pursuing raw input, we reorganized the PhysNet model with the Temporal Normalization Module in our new work: https://github.com/KegangWangCCNU/TemporalNormalization/blob/main/PhysNet_TN_rPPG-Toolbox.py. This method could achieve great performance in the raw-to-raw pipeline. So I am closing this PR but still open for any new discussion in the issues. |
No description provided.