Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise FIRuncFilter full covariance #190

Merged
merged 25 commits into from Feb 16, 2021
Merged

Conversation

mgrub
Copy link
Contributor

@mgrub mgrub commented Nov 20, 2020

This pull request addresses issue #175 in some way.

By introducing a new function _fir_filter we can propagate full covariance information into the output of an FIR-filter. Based on this function a wrapper FIRuncFilter_2 is introduced, mimicing the behaviour of the existing FIRuncFilter. (And thus preparing a potential replacement lateron.)

Benefits of the new function(s):

  • input can now have full covariance information as well
  • return full covariance of output
  • reduced complexity of the code
  • no more python-for loops (all is done using 2D-convolution)
  • reduce computations if Utheta == None or Ux is None (this was already done for the FIRuncFilter, but in a much more complex way)
  • control over initial conditions of the FIR filter (at least limited)

A visual comparison with Monte Carlo covariance result show good agreement.
comparison_fir_mc

TODO: A quick runtime comparison of both methods should be done to evaluate potential performance gains/losses.

@mgrub mgrub changed the title Revise fi runc filter full covariance Revise FIRuncFilter full covariance Nov 23, 2020
fixes issue with bad condition number of covariance
fixes issue of negative main diagonal in case="corr"
(because resulting toeplitz matrix is not guaranteed to be positive semidefinite, which is bad for a covariance)
@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #190 (9c9d77a) into master (69e42ad) will increase coverage by 0.87%.
The diff coverage is 65.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   44.60%   45.48%   +0.87%     
==========================================
  Files          23       23              
  Lines        1641     1728      +87     
  Branches      285      313      +28     
==========================================
+ Hits          732      786      +54     
- Misses        834      852      +18     
- Partials       75       90      +15     
Impacted Files Coverage Δ
PyDynamic/__init__.py 100.00% <ø> (ø)
PyDynamic/uncertainty/propagate_filter.py 60.30% <65.00%> (-0.44%) ⬇️
PyDynamic/uncertainty/__init__.py 100.00% <100.00%> (ø)
PyDynamic/uncertainty/propagate_MonteCarlo.py 52.94% <0.00%> (+0.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69e42ad...9c9d77a. Read the comment docs.

PyDynamic/uncertainty/propagate_filter.py Outdated Show resolved Hide resolved
PyDynamic/uncertainty/propagate_filter.py Outdated Show resolved Hide resolved
@mgrub
Copy link
Contributor Author

mgrub commented Nov 27, 2020

@BjoernLudwigPTB and I already had a in-depth look at the code structure, tweaked docstrings and discussed signatures of the new functions.

As of now, the new FIRuncFilter_2 can be used as a drop-in replacement for FIRuncFilter. All cases that FIRuncFilter can cover are identically reproduced by FIRuncFilter_2 in terms of input-output behavior. Runtime-performance is similar (but not identical, some cases are handled slightly worse, other are handled better) - but we catch some special cases that FIRuncFilter handled very well to keep up with the old performance. Moreover, FIRuncFilter_2 can now handle the following additional scenarios:

  • sigma_noise is full covariance
  • Utheta is float or diagonal
  • output uncertainty can be returned as full covariance

During our discussion, we were thinking of how to proceed from here. We would propose to let FIRuncFilter_2 take the role of FIRuncFilter and move the old implementation into testing to maintain compatibility. We were thinking of:

  1. publish as-is in a minor-release (no breaking changes)
  2. change default of return_full_covariance to True (which is a breaking change) and publish in next major release
  3. make the shape of Uy dependent on the dimension of sigma_noise (which is a breaking change) and publish in next major release

@eichstaedtPTB : What do you think?

@eichstaedtPTB
Copy link
Collaborator

My suggestion is to

  • set return_full_covariance to False to maintain backwards compatibility
  • issue a (subtle) warning to the user whenever the output uncertainty would have been a full covariance matrix and suggest setting return_full_covariance to True

@mgrub
Copy link
Contributor Author

mgrub commented Feb 15, 2021

Alright, I made some final touches to this PR:

  • add subtle note to user in case, that output of an FIR filter basically always has full covariance matrix:

FIRuncFilter: Output uncertainty will be given as 1D-array of point-wise
standard uncertainties. Although this requires significantly lesser computations,
it ignores correlation information. Every FIR-filtered signal will have
off-diagonal entries in its covariance matrix (assuming the filter is longer
than 1). To get the full output covariance matrix, use 'return_full_covariance=True'.

  • rename FIRuncFilter to legacy_FIRuncFilter and move to test/test_propagate_filter.py
  • rename FIRuncFitler_2 to FIRuncFilter
  • adjust Monte-Carlo test test_FIRuncFilter_MC_uncertainty_comparison to compare full covariance output
  • compare new implementation to legacy implementation in test_FIRuncFilter_legacy_comparison
  • pull master (without conflicts)

@mgrub mgrub merged commit 9897435 into master Feb 16, 2021
@mgrub mgrub deleted the revise_FIRuncFilter_full_covariance branch February 16, 2021 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants