-
Notifications
You must be signed in to change notification settings - Fork 298
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
[WIP] add HOEDMD #408
base: master
Are you sure you want to change the base?
[WIP] add HOEDMD #408
Conversation
Hi @jieli-matrix, thanks a lot for your contribution! Could you please format your code with black ? |
pydmd/hoedmd.py
Outdated
As a reference consult this work by Weiyang and Jie: | ||
https://doi.org/10.1137/21M1463665 | ||
""" | ||
from __future__ import division |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this import
Code format & remove import done. cc: @fAndreuzzi @mtezzele |
reformat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jieli-matrix, I've left some comments. They are mainly related to code style, for instance in PyDMD we try to simplify indexing expressions (you've some quite complex ones) by skipping the trailing :
when not needed. Could you please fix all the occurrences in your code?
I'll finish reviewing the PR after you're ready for the next round. Thanks!
pydmd/hoedmd.py
Outdated
self._compute_modes(U, V, inprodUV) | ||
|
||
else: | ||
raise ValueError("Invalid value for alg_type: {}".format(alg_type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise ValueError("Invalid value for alg_type: {}".format(alg_type)) | |
raise ValueError(f"Invalid value for alg_type: {alg_type}") |
|
||
:param numpy.ndarray Psi: the time-delay data matrix containing the snapshots x0,..x{n} by column. | ||
:param int d: order in HOEDMD. | ||
:param string alg_type: Algorithm type in HOEDMD. Default is 'stls'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document briefly all possible values for alg_type
.
pydmd/hoedmd.py
Outdated
Px = P[:-d, :] | ||
Py = P[d:, :] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the trailing :
Px = P[:-d, :] | |
Py = P[d:, :] | |
Px = P[:-d] | |
Py = P[d:] |
pydmd/hoedmd.py
Outdated
self._compute_eigenquantities() | ||
U = self.eigenvectors_r | ||
V = self.eigenvectors_l | ||
inprodUV = self.eigenvalues.conj() * ((U.conj() * V).sum(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inprodUV = self.eigenvalues.conj() * ((U.conj() * V).sum(0)) | |
inprodUV = self.eigenvalues.conj() * (U.conj() * V).sum(0) |
pydmd/hoedmd.py
Outdated
U = self.eigenvectors_r | ||
V = self.eigenvectors_l | ||
inprodUV = self.eigenvalues.conj() * ((U.conj() * V).sum(0)) | ||
U = P[m : 2 * m, :].dot(U) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the trailing :
U = P[m : 2 * m, :].dot(U) | |
U = P[m : 2 * m].dot(U) |
pydmd/hoedmd.py
Outdated
Gxx = np.dot(Psi[:-d, :].T.conj(), Psi[:-d, :]) | ||
Gxy = np.dot(Psi[:-d, :].T.conj(), Psi[d:, :]) | ||
Gpp = np.dot(Psi[m * d :, :].T.conj(), Psi[m * d :, :]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above:
Gxx = np.dot(Psi[:-d, :].T.conj(), Psi[:-d, :]) | |
Gxy = np.dot(Psi[:-d, :].T.conj(), Psi[d:, :]) | |
Gpp = np.dot(Psi[m * d :, :].T.conj(), Psi[m * d :, :]) | |
Gxx = np.dot(Psi[:-d].T.conj(), Psi[:-d]) | |
Gxy = np.dot(Psi[:-d].T.conj(), Psi[d:]) | |
Gpp = np.dot(Psi[m * d :].T.conj(), Psi[m * d :]) |
pydmd/hoedmd.py
Outdated
Gpp = np.dot(Psi[m * d :, :].T.conj(), Psi[m * d :, :]) | ||
sigma, Q = np.linalg.eig(Gxx + Gpp) | ||
ind = np.sort(-sigma) | ||
sigma = sigma(ind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify what this part is doing? Why are you using round brackets to index sigma
? Is it running without errors?
pydmd/hoedmd.py
Outdated
self._compute_eigenquantities() | ||
U = self.eigenvectors_r | ||
V = self.eigenvectors_l | ||
inprodUV = self.eigenvalues.conj() * ((U.conj() * V).sum(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inprodUV = self.eigenvalues.conj() * ((U.conj() * V).sum(0)) | |
inprodUV = self.eigenvalues.conj() * (U.conj() * V).sum(0) |
pydmd/hoedmd.py
Outdated
U = Psi[m : 2 * m, :] * (Q * U) | ||
V = Psi[: m * d, :] * ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
U = Psi[m : 2 * m, :] * (Q * U) | |
V = Psi[: m * d, :] * ( | |
U = Psi[m : 2 * m] * (Q * U) | |
V = Psi[: m * d] * ( |
Hi @jieli-matrix, I suggest focusing on the tests as the main priority so we can merge this PR. Later you can add the Guassian kernel option and the tutorial showcasing your method. |
Hi @jieli-matrix, could you please update us on this PR? |
Sorry for late response! I will update this PR in this week. cc: @mtezzele |
@jieli-matrix you need to rebase this branch and fix the conflicts (see |
@jieli-matrix Do you have any updates on this PR? |
Sorry for update. I will add all tests in this week. |
Description:
Add a new dmd-variant for pyDMD, HOEDMD. Here is the paper link and the Matlab implementation version can be found in jieli-matrix/HOEDMD. We extended HODMD to HOEDMD and designed efficient numerical implementation by solving the Structured Total Least Squares problem. Thus, the implementation of HOEDMD is quite similar to HODMD overall while we need to rewrite the DMDOperator part.
ToDo:
Help:
Weiyang Ding and I want to thank PyDMD because it is convenient to compare our method with the existing DMD variants. We hope to add HOEDMD to PyDMD though we have developed HOEDMD in Matlab. Since I am a new developer at PyDMD, please feel free to comment on the bugs and give guidelines. Thanks!