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

PyDMD based DMD ROMs #2342

Open
wants to merge 38 commits into
base: devel
Choose a base branch
from
Open

PyDMD based DMD ROMs #2342

wants to merge 38 commits into from

Conversation

alfoa
Copy link
Collaborator

@alfoa alfoa commented Jul 25, 2024


Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)

Closes #2343

What are the significant changes in functionality due to this change request?

Addition of pydmd library for the importing of several DMD based algorithms

Created interface with RAVEN ROMs

And automatic documentation

And associated tests


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added is the the analytic documentation updated/added?
  • 9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

@alfoa alfoa changed the title Draft: PyDMD based DMD ROMs PyDMD based DMD ROMs Jul 26, 2024
@moosebuild
Copy link

Job Test CentOS 8 on 7470d51 : invalidated by @alfoa

2 similar comments
@moosebuild
Copy link

Job Test CentOS 8 on 7470d51 : invalidated by @alfoa

@moosebuild
Copy link

Job Test CentOS 8 on 7470d51 : invalidated by @alfoa

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 7470d51 : invalidated by @alfoa

@moosebuild
Copy link

Job Mingw Test on 7470d51 : invalidated by @alfoa

@aalfonsi
Copy link
Collaborator

aalfonsi commented Aug 1, 2024

@wangcj05 @joshua-cogliati-inl @Jimmy-INL FY: if you can send feedbacks within tomorrow I can try to address them before leaving on Friday. Otherwise it will need to wait till September.

@alfoa
Copy link
Collaborator Author

alfoa commented Aug 2, 2024

I will leave today. So this MR will need to wait to September.

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 7470d51 : invalidated by @Jimmy-INL

@moosebuild
Copy link

Job Test CentOS 8 on 7470d51 : invalidated by @Jimmy-INL

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 7470d51 : invalidated by @Jimmy-INL

1 similar comment
@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 7470d51 : invalidated by @Jimmy-INL

@moosebuild
Copy link

Job Test CentOS 8 on 7470d51 : invalidated by @joshua-cogliati-inl

Cleaned up files on test machine.

@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 7470d51 : invalidated by @joshua-cogliati-inl

Cleaned up files on test machine.

1 similar comment
@moosebuild
Copy link

Job Test Ubuntu 18 PIP on 7470d51 : invalidated by @joshua-cogliati-inl

Cleaned up files on test machine.

@alfoa
Copy link
Collaborator Author

alfoa commented Aug 21, 2024

@wangcj05 @Jimmy-INL can anyone review this? I will fly back on Saturday and I would like to close this if possible.

@alfoa alfoa requested review from mandd and removed request for wangcj05 and Jimmy-INL August 27, 2024 14:29
Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alfoa I have a couple of comments for you to consider.

Comment on lines 49 to 68
self._dynamicHandling = True
self.settings = None # initial settings for the ROM
self.dmdParams = {}
# parametric model
self.model = None # ParametericDMD
# local models
## POD
self._DRrom = None
## RBF
self._interpolator = None
## base specific DMD estimator/model (Set by derived classes)
self._dmdBase = None
## DMD fit arguments (overloaded by derived classes (if needed))
self.fitArguments = {}
# flag to indicate that the model has a single target (in addition to the pivot parameter)
# This flag is needed because the DMD based model has an issue with single target (space dimension == 1) and
# a counter mesurament (concatenation of snapshots) is required
self.singleTarget = False
# target indeces
self.targetIndeces = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please provide some comments for the self variables

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comments are above the variables

self.model = None # ParametericDMD
# local models
## POD
self._DRrom = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelBack here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed name

# a counter mesurament (concatenation of snapshots) is required
self.singleTarget = False
# target indeces
self.targetIndeces = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean indices here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

"""
specs = super().getInputSpecification()
specs.addSub(InputData.parameterInputFactory("light", contentType=InputTypes.BoolType,
descr=r"""TWhether this instance should be light or not. A light instance uses
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change TWhether to Whether?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

settings, notFound = paramInput.findNodesAndExtractValues(['pivotParameter','light', 'reductionRank', 'reductionMethod'])
# notFound must be empty
assert(not notFound)
self.settings = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this variable is set to None in the init method, do you have specific reason for that? Should the default to dict?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is (line 50)

Created on July 21, 2024

@author: Andrea Alfonsi
Kernalized Dynamic Mode Decomposition
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, change 'Kernalized' to Kernelized

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 148 to 149
#for target in set(self.target) - set(self.pivotID):
# self._dmdBase[target] = EDMD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 26 to 28
np = importModuleLazy("numpy")
pydmd = importModuleLazy("pydmd")
ezyrb = importModuleLazy("ezyrb")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

super().__init__()

# local model
self._dmdBase = None #{} # FbDMD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may not need it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines +170 to +194
# if 0.0 < float < 1.0, computed rank is the number of the biggest sv needed to reach the energy identified by this float value
self.dmdParams['svd_rank'] = settings.get('svd_rank')
# truncation rank for total least square
self.dmdParams['tlsq_rank'] = settings.get('tlsq_rank')
# True if the exact modes need to be computed (eigs and eigvs), otherwise the projected ones (using the left-singular matrix)
self.dmdParams['exact' ] = settings.get('exact')
# amplitudes computed minimizing the error between the mods and all the timesteps (True) or 1st timestep only (False)
self.dmdParams['opt'] = settings.get('opt')
# Rescale mode
self.dmdParams['rescale_mode'] = settings.get('rescale_mode')
if self.dmdParams["rescale_mode"] == 'None':
self.dmdParams["rescale_mode"] = None
# Sorted eigs
self.dmdParams['sorted_eigs'] = settings.get('sorted_eigs')
if self.dmdParams["sorted_eigs"] == 'False':
self.dmdParams["sorted_eigs"] = False
# Forward Backward method (see FbDMD)
self.dmdParams['forward_backward'] = settings.get('forward_backward')
# Sorted eigs
self.dmdParams['d'] = settings.get('d')
# Reconstruction method
self.dmdParams['reconstruction_method'] = settings.get('reconstruction_method')
# svd_rank_extra
self.dmdParams['svd_rank_extra'] = settings.get('svd_rank_extra')

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use a for loop here to update self.dmdParams here. This suggestion will apply to other classes as well.

@@ -90,6 +90,8 @@ Note all install methods after "main" take
<!-- source="mamba" are the ones installed when mamba is installed -->
<mamba source="mamba" skip_check='True'/>
<serpentTools optional='True' source="pip"/>
<pydmd source="pip"/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the license of these dependencies being checked out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. MIT license. Compatible with APACHE-2

@@ -2314,6 +2325,60 @@ \subsubsection{SyntheticHistory}
Specifies the degree polynomial to fit the data with.
\end{itemize}

\item \xmlNode{autoarma}:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not part of this PR right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. it is generated by the automatic documentation script. It seems that that parameter has been added in the SyntheticHistory object but the manual never got updated.

@ In, paramInput, ParameterInput, the already parsed input.
@ Out, None
"""
import pydmd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be move at the beginning of the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done here so we can keep the "lazy import" on the top of the file (to avoid to import the library if an object depending on it is not used in the input file)....it should accelerate the initialization of the framework (for cases where the dmd is not used)

@ In, paramInput, ParameterInput, the already parsed input.
@ Out, None
"""
import pydmd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

# amplitudes computed minimizing the error between the mods and all the timesteps (True) or 1st timestep only (False)
self.dmdParams['opt'] = settings.get('opt')
# seed
import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

# intialize the model
self.initializeModel(settings)

def _preFitModifications(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method is not present in other classes, is there a reason for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. This is only implemented for classes that require a direct access to the independent variable (i.e. time).
The method is needed only for few classes.

0.075,55.2114371811,0.000184038120394
0.1,23.7800706056,7.92669004928e-05
0.2,10.2422937507,3.41409785118e-05
0.3,4.41144952912,1.47048314807e-05
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gold file looks fairly different, any comment here?

# amplitudes computed minimizing the error between the mods and all the timesteps (True) or 1st timestep only (False)
self.dmdParams['opt'] = settings.get('opt')
# seed
import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

super().__init__()

# local model
self._dmdBase = None #{} # DMD
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be an empty dict?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

from ...utils import InputData, InputTypes
#Internal Modules End--------------------------------------------------------------------------------

class DMD(DMDBase):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what is the scope of this DMD class

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is the base DMD. (original formulation of the DMD method)

@alfoa
Copy link
Collaborator Author

alfoa commented Sep 3, 2024

@wangcj05 @mandd addressed your comments.

@alfoa alfoa requested review from mandd and wangcj05 September 3, 2024 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK] Inclusion of several DMD-based approach through PyDMD
5 participants