[Re] The Discriminative Kalman Filter for Bayesian Filtering with Nonlinear and Non-Gaussian Observation Models

[Josuelmet]

Original article: The Discriminative Kalman Filter for Bayesian Filtering with Nonlinear and Non-Gaussian Observation Models

PDF URL: https://github.com/Josuelmet/Discriminative-Kalman-Filter-4.5-Python/blob/main/article/article.pdf
Metadata URL: https://github.com/Josuelmet/Discriminative-Kalman-Filter-4.5-Python/blob/main/article/metadata.yaml
Code URL: https://github.com/Josuelmet/Discriminative-Kalman-Filter-4.5-Python/tree/main

Scientific domain: Computational Neuroscience
Programming language: Python
Suggested editor:

[rougier]

Very sorry for the late answer. We'll assign an editor soon.

[rougier]

@benoit-girard @oliviaguest @gdetor Can any of you handle this submission?

[benoit-girard]

Sorry for this much delayed answer: yes, I think I can handle this submission.

[benoit-girard]

@junpenglao : would you be interested in reviewing this submission?

[benoit-girard]

@rchavarriaga : would you be interested in reviewing this submission?

[benoit-girard]

@Josuelmet : do you have ideas of reviewers you could suggest? I have difficulties identifying who, in the usual Rescience C reviewers, could do the job adequately.

[Josuelmet]

@benoit-girard Sure,
I think @ozancaglayan @mxochicale @gdetor would be good picks, since they are familiar with signal processing and computational neuroscience.

[benoit-girard]

@ozancaglayan @mxochicale @gdetor : would any of you be interested in reviewing this submission?

[ozancaglayan]

Hello. I'm no longer in academia and have limited time but I'll see if I can do it if nobody else volunteers.

[benoit-girard]

@ozancaglayan Your status wrt. academia is not a problem (we don't have such requirements at ReScience C), as long as you have the expertise and know-how to perform the review.
It would be a pleasure to have you doing it, as I have been unsucessfully looking for reviewers so far.

[ozancaglayan]

Okay, you can assign this to me

[ozancaglayan]

Review

First of all, I'd like to thank the authors as the paper is very clear and fun to read! The authors went for replicating the experiment 4.5 which they claim to be the most significant type of experiment for BCI studies. The replication study is able to confirm the trends shown by the original authors with one method underperforming and another outperforming the original results.

I was particularly curious about the LSTM/NN approach which often requires quite some tuning to get the best out of it. I'm pretty sure it could have surpassed the other methods given enough time and patience. Authors here already show that by changing a couple of settings, they were able to get better performance than the claimed ones.

Unfortunately, I'm having issues in running the experiments so let's just first focus on making this work and then I'll follow up with the replication status.

Test environments:

• Python 3.12 w/ Mambaforge + pip install -r requirements.txt (Ubuntu 22.04.3, x86_64) ❌
• Same as above but on Mac M1 fails ❌ (see below, this is more of a technical issue which should just be mentioned)

Readability

• It would be very beneficial for readability if you use black, isort and friends to lint and format your .py scripts in the root repository.

• Please clear the cell outputs from the notebook files a bit. There are lots of warnings printed out there cluttering the actual output, you can use with np.errstate(divide='ignore', invalid='ignore'): wrappers around expected division issues:

  f_est = xP / sP
C:\Users\josue\Documents\Python Scripts\DKF\Python_Paper_45\optimize_sx.py:42: RuntimeWarning: Mean of empty slice
  return np.nanmean((xtrain - preds) ** 2)

Issues

• sklearn dependency should be renamed to scikit-learn in requirements.txt

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [15 lines of output]
      The 'sklearn' PyPI package is deprecated, use 'scikit-learn'
      rather than 'sklearn' for pip commands.

• There seems to be an issue on Macs: Jupyter kernel crashes. When I launch it from the CLI as a script, I get a warning & error regarding Hint This means that multiple copies of the OpenMP runtime have been linked into the program.. There are some suggestions regarding this and one suggestion was to switch the imports for torch and numpy which seemed to make the error go away but now the notebook stuck at Iteration 0. Created a clean conda environment and installed the nomkl package first, and then the dependencies. Now I get a segmentation fault. It may be beneficial to add a note regarding this in README and recommend people to use x86_64 architecture machines.

• I now switched to Linux. When running the actual Paper replication script, I get this:

Iteration 0:
DKF-NN
/home/ozan.caglayan/Discriminative-Kalman-Filter-4.5-Python/optimize_sx.py:43: RuntimeWarning: Mean of empty slice
  return np.nanmean((xtrain - preds) ** 2)
DKF-GP
/home/ozan.caglayan/Discriminative-Kalman-Filter-4.5-Python/optimize_sx.py:43: RuntimeWarning: Mean of empty slice
  return np.nanmean((xtrain - preds) ** 2)
Traceback (most recent call last):
  File "/home/ozan.caglayan/Discriminative-Kalman-Filter-4.5-Python/Paper_Script_45.py", line 268, in <module>
    zpred.loc[iRun, "DKF_GP"] = DKF_filtering(
                                ^^^^^^^^^^^^^^
  File "/home/ozan.caglayan/Discriminative-Kalman-Filter-4.5-Python/DKF_filtering.py", line 30, in DKF_filtering
    if not np.all(eig(inv(bb) - inv(VZ))[0] > 0):
                  ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ozan.caglayan/mambaforge/envs/kalman/lib/python3.12/site-packages/numpy/linalg/linalg.py", line 1329, in eig
    _assert_finite(a)
  File "/home/ozan.caglayan/mambaforge/envs/kalman/lib/python3.12/site-packages/numpy/linalg/linalg.py", line 218, in _assert_finite
    raise LinAlgError("Array must not contain infs or NaNs")
numpy.linalg.LinAlgError: Array must not contain infs or NaNs

Other

• You have the .npy files committed to the flint-data-preprocessing subfolder. Do we want this? As you already provide the pre-processing logic with freshly downloaded MAT files, it makes it a bit confusing for the user to understand what to expect here. Should they pre-process from scratch or use the provided files?

• In flint-data-preprocessing/README.md, you instruct the reader to move the .npy files to the root folder but further ipynb notebooks actually access them from the flint-data-preprocessing subfolder which makes sense. Could you remove that instruction from the README file maybe for clarity?

[Josuelmet]

Thanks for responding @ozancaglayan !
I'll get back to you ASAP, sorry for the delay in my response.

[benoit-girard]

@ozancaglayan Thanks a lot for your review.

[benoit-girard]

I am asking again with the hope this notification will succeed: @mxochicale @gdetor : would any of you be interested in reviewing this submission?

[Josuelmet]

@ozancaglayan I updated the repo a bit, here is my progress:

• Replaced sklearn with scikit-learn in requirements.txt
• Updated preprocessing README to say that running preprocessing is optional and that the processed files should exist in the flint-data-preprocessing folder.
• Switched torch and numpy import order in Paper_Script_45.ipynb
• Updated the main README to suggest using x86_64 architectures
• Run Paper_Script_45.ipynb in a Colab runtime to make sure code works in a Linux environment.
• Clear / trim up cell outputs from notebooks.

I will let you know when I have cleared the Linux issues.
Thanks!

Edit: I have cleared the Linux issues in the new code as of 3/6/2024.

[Josuelmet]

@ozancaglayan I have figured out the issue behind the instability, and there are two ways to fix it:

• either use scipy==1.10.1 , which is what I realized I used on my system; or,
• use the newest version of the code/repo (with any version of scipy, including the latest), in which I have tweaked the initialization of an important kernel parameter to avoid instability / nans.

I recommend the latter; let me know when you are able to give running the code another chance.
Thanks!

P.S. I ran the code for one seed (42) in a new Linux environment (Colab); some of the numbers for Table 2 jittered a bit, but overall the trends were the same. The new numbers matched Table 3 very closely, with some jitter in the DKF-NN row.

[Josuelmet]

@ozancaglayan were you able to run the code again?

[ozancaglayan]

Sorry for the delay, I'm on leave with no access to computer. Will look into after July 15.

[ozancaglayan]

Hi,

• Went back to the codebase and I can confirm that the notebook is now running in Linux x86_64 without any issues. I tried with the recent version of scipy without pinning it to 1.10.1 and it seems to work fine as well ✅
• All my previous points were addressed by the author ✅

Remarks about the results in Table 2

• Kalman, GP, LSTM and EKF results are exactly as they are given in the Table
• DKF-NW, DKF-NN and UKF have some variations across the trials. What's interesting is that the variations are not happening at each trial but only in some of them, which is a bit weird. Any idea on what may be the reason here? These variations slightly change the trial averages as well. See the below figure where I wrote in red the numbers that I'm getting from your notebook:

(I would have attributed this to some torch/NN related non-determinism but given that LSTM results are exactly the same as the table, I think there's sth else going on here. To verify, I ran the notebook twice, the table differences are consistent, e.g. they don't change across runs)

Remarks about the results in Table 3

• DKF-NN and UKF have some variations again, non-negligible differences for DKF-NN. (Now the variations for DKF-NN happened at each trial. Since these are two different evaluation metrics, I would have expected the diffs also happening in the first table but didn't check whether the scores change slightly all the time or not.)

Some minor/further improvements:

• You can set a global precision for rendering the tables: pd.set_option('display.precision', 3)
• Could you rename table1 and table2 DataFrame's in the notebook to table2 and table3 so that they match the table numbers in your paper?
• Could you fix the below issues of setting a string to a float column, which would cause errors in the future so that your pipeline is ensured to be correctly executed by future versions of pandas? I think you can simply remove this and multiply the 0-1 improvement scores by 100 and with the display.precision fix, they'll show up as -18.7 for example which should be fine

[/tmp/ipykernel_387075/859728690.py:10](http://10.98.58.43:8001/tmp/ipykernel_387075/859728690.py#line=9): FutureWarning: Setting an item of incompatible dtype is deprecated and will raise an error in a future version of pandas. Value '-19%' has dtype incompatible with float64, please explicitly cast to a compatible dtype first.
  table1.loc[idx] = table1.loc[idx].map(lambda n: '{:.0%}'.format(n))

Conclusion

@Josuelmet I'm curious about your take on the result differences. Thanks!

[Josuelmet]

@ozancaglayan I took a deep look at the differences between two runtimes (one on my PC, and one on Colab), and even when using the same version of PyTorch, I was unable to make the two sets of results fully agree, particularly for the simple NN-based architectures: DKF-NN, EKF/UKF. My best guess is that this difference stems from somewhere within PyTorch. The LSTM seems to be more stable since it runs for much fewer epochs than the DKF-NN, EKF, and UKF. Between my two runtimes, I looked at the NN losses, and for some reason they would agree fully up until 330 epochs, after which they diverged. As a result, I tried modifying the training parameters to use fewer epochs, but in the end I was unable to fully remove all discrepancies between my two runtimes. Let me know what you think. Thanks!

[rougier]

@benoit-girard Any progress on that ?

[Josuelmet]

Hi everyone!
Just checking in. @benoit-girard @ozancaglayan any thoughts?
Thanks!

[ozancaglayan]

I had some minor remarks in my last comment. Besides the inconsistencies in a small part of the results that we discussed, I'm fine with this, sorry for the delays!