SamProell / yarppg

Yet another implementation of remote photoplethysmography (rPPG) in Python
MIT License
51 stars 13 forks source link

Update for LiCVPR processor.py #10

Open blank-ed opened 2 years ago

SamProell commented 2 years ago

Hi @blank-ed thank you very much for contributing. Could you please create a single PR with all changed files? Not sure if you wanted me to review already, since the PRs are marked as drafts.

Commenting here anyways on a couple of things I spotted when going through your pull requests (PR 7-10) :

Hope this helps. Let me know if you want to discuss something in more detail!

blank-ed commented 2 years ago

Hi @SamProell. I'm not really familiar with GitHub, and I tried combining them in a single PR but I couldn't do it 😂. Please let me know how to do this, thank you! Also, below the This branch has no conflicts with the base branch, it says "Only those with write access to this repository can merge pull requests". So I think you might be able to merge all the 4 PR's? Please let me know about this, thank you.

Oh yeah, sorry, I didn't know that drafts couldn't be reviewed. My bad. I will mark them ready to be reviewed.

Your comments definitely helped. Thank you!

Also, I have a question regarding LiCVPR, especially the NLMS adaptive filter. So from the paper's implementation of the NLMS filter, it goes something like this:

image

They did not specify the initial value of the h at j=0, which is h(0) along with the initial value of gIR at j=0, which is gIR(0). As far as I understand, we need initial value of h (which is h(0)) in order to calculate the initial value of gIR(0), so that we can obtain h(1) and so on. Since they have provided the formula of how the NLMS filter is used, do we just assume our initial value of h(0) to be 0, or am I heading in the wrong direction?

SamProell commented 2 years ago

@blank-ed, I am not sure what the problem is. In your fork, you would need to create new branch, commit and push changes to all files and then create the PR from the new branch. I quickly tested this with a different account and it worked without any problems. Once we have a single pull requests with all changes, I will review it more thoroughly and merge it into yarppg when it's ready. It is important to have a single PR, so that we can test the final functionality after all the changes.

I added the bgmask exactly for this, but I never managed to complete the LiCVPR method. So your contribution is more than welcome. Yes, a zero initialization should be the way to go. Although it is not explicitly mentioned in the paper. The NLMS algorithm described in wikipedia uses zeros as well.