PyDMD
Fixed bug in building convolution windows for mrCOSTS
Hi all,
I found a small bug in how mrCOSTS builds it convolution windows and performed window rounding. The long story short was that I didn't correctly adapt the original MATLAB indexing for python. This pull request fixes that issue.
Summary of changes:
- Convolution windows had MATLAB style indexing and not python. As a result the windows did not peak at the right place.
- Similarly, the window rounding was asymmetric.
- Causes slight reduction in the reconstructed error in the toy data example.
-
bamplitudes now fall along PSD of the input data, fixing a lingering issue in the mrCOSTS fit and giving it more diagnostic power. - Fixed the mrCOSTS tests not correctly tearing down.
Thanks for taking a look! Karl
Just as a public note, the lv_kern function still returns an asymmetric kerning ((np.arange(0, window_length) - window_length - 1) should be (np.arange(0, window_length) - window_length + 1) ). Unfortunately, making the window symmetric seems to reduce the fit quality in testing, so I am just accepting that this is a weird idiosyncrasy of the method for the time being.
I managed to break everything.
The tutorial tests failing is Notebook 2 (Advanced DMD) because scipy.integrate.trapz wasn't found. The most recent release of scipy removed .trapz in favor or .trapezoid (https://docs.scipy.org/doc/scipy/release/1.14.0-notes.html). Do we want to pin the scipy version to be >=1.14 and update the call to the non-deprecated version?
I think the unit tests broke because I did os.chdir(tmp_path) in the mrCOSTS unit tests. Then the other tests were pointing to the wrong directory when it came time for them to load the test data. I don't have the internet connection to download the test extras so I'm testing this directly here on github instead of locally.
I managed to break everything.
The tutorial tests failing is Notebook 2 (Advanced DMD) because scipy.integrate.trapz wasn't found. The most recent release of scipy removed
.trapzin favor or.trapezoid(https://docs.scipy.org/doc/scipy/release/1.14.0-notes.html). Do we want to pin the scipy version to be >=1.14 and update the call to the non-deprecated version?
Thank you for pointing this out. @fandreuz @ndem0 @sichinaga Should we add a try/else since version 1.14 is the latest one? I don't want to limit the current users. IMHO we can impose the version after 1 or 2 new versions. What do you think?
@mtezzele I propose to keep only the new version, scipy.integrate.trapezoid has been around for 4 years: https://github.com/scipy/scipy/blob/v1.6.0/scipy/integrate/_quadrature.py
@mtezzele I propose to keep only the new version,
scipy.integrate.trapezoidhas been around for 4 years: https://github.com/scipy/scipy/blob/v1.6.0/scipy/integrate/_quadrature.py
Ok, so we can enforce scipy >= version_of_4_years_ago instead of the current one, which is quite new. What do you think?
@mtezzele I propose to keep only the new version,
scipy.integrate.trapezoidhas been around for 4 years: https://github.com/scipy/scipy/blob/v1.6.0/scipy/integrate/_quadrature.pyOk, so we can enforce scipy >= version_of_4_years_ago instead of the current one, which is quite new. What do you think?
Yes looks good! You want to do it? Otherwise I'll open a PR
@mtezzele I propose to keep only the new version,
scipy.integrate.trapezoidhas been around for 4 years: https://github.com/scipy/scipy/blob/v1.6.0/scipy/integrate/_quadrature.pyOk, so we can enforce scipy >= version_of_4_years_ago instead of the current one, which is quite new. What do you think?
Yes looks good! You want to do it? Otherwise I'll open a PR
Go ahead please! Thank you very much!