-
Notifications
You must be signed in to change notification settings - Fork 37
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
Parametric dmdc heat conduction #264
Conversation
There is a lot of file output that is hard to delete. It would be more convenient to use a subdirectory for file output, as done in examples/dmd/de_dg_advection_greedy.cpp (search for |
lib/algo/DMDc.h
Outdated
@@ -204,10 +238,11 @@ class DMDc | |||
* @param[in] dt d_dt | |||
* @param[in] t_offset d_t_offset | |||
* @param[in] state_offset d_state_offset | |||
* @param[in] basis d_basis |
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.
d_basis
-> basis
For backwards compatibility, should this have default value basis = nullptr
? Before this PR, the member d_basis
was set by the class, not the user. In what cases should this be input, and why is not overwritten by the class? Some documentation on this would be helpful.
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.
Good point! I changed the line of code to match your suggestion and added some documentation in the hpp file (maybe it would be more helpful elsewhere?)
The idea is that for offline DMDc we compute the basis. For the interpolated DMDc we interpolate the basis, and then we need to store it so that we can later project the controls (see line 678 in DMDc.cpp
)
lib/algo/ParametricDMDc.h
Outdated
dmdcs[0]->d_k,dmdcs[0]->d_dt, | ||
dmdcs[0]->d_t_offset, dmdcs[0]->d_state_offset, W); | ||
|
||
// delete W; |
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.
Decide whether these pointers should be deleted?
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.
I keep both of them. Added a commit to delete the commented lines
The new commit adds an output directory using the same "io_dir" code as in the other example. |
|
||
if (offline) | ||
{ | ||
delete dmd_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.
I think dmd_u
can be deleted for the online case as well, since it is allocated in getParametricDMDc()
.
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.
We should introduce smart pointers in libROM (unique_ptr
, shared_ptr
), which is beyond the scope of this PR. For now, let's delete pointers as needed, but I think we have too many pointer issues and need to switch to smart pointers.
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're right, dmd_u
is now always deleted
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.
@dylan-copeland I definitely agree, we should probably open an issue for switching to smart pointers so we remember to come back to it in the future.
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.
Thanks @andersonw1! The changes look good overall. I only have some minor comments in the formats. I am also curious if we can do a fair comparison between DMD and DMDc on the same example with external source, which we should expect DMDc works better.
lib/algo/DMDc.cpp
Outdated
@@ -107,7 +107,7 @@ DMDc::DMDc(std::string base_file_name) | |||
|
|||
DMDc::DMDc(std::vector<std::complex<double>> eigs, Matrix* phi_real, | |||
Matrix* phi_imaginary, Matrix* B_tilde, int k, | |||
double dt, double t_offset, Vector* state_offset) | |||
double dt, double t_offset, Vector* state_offset,Matrix* basis) |
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.
Add a space before Matrix
?
lib/algo/DMDc.cpp
Outdated
@@ -561,6 +569,7 @@ DMDc::constructDMDc(const Matrix* f_snapshots, | |||
// Calculate the projection initial_condition onto column space of d_basis. | |||
project(init, f_controls); | |||
|
|||
|
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.
Redundant line.
lib/algo/DMDc.h
Outdated
* @param[in] parameter_points The training parameter points. | ||
* @param[in] dmdcs The DMD objects associated with | ||
* each training parameter point. | ||
* @param[in] controls The matrices of controls from previous runs which we use to interpolate. |
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.
Non-uniform indents
lib/algo/ParametricDMDc.h
Outdated
* @param[in] dmdcs The DMDc objects associated with | ||
* each training parameter point. | ||
* @param[in] controls The matrices of controls from previous runs which we use to interpolate. | ||
* @param[in] controls_interpolated The interpolated controls. |
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.
Non-uniform indents
lib/algo/ParametricDMDc.h
Outdated
* @param[in] dmdc_paths The paths to the saved DMD objects associated with | ||
* each parameter point. | ||
* @param[in] desired_point The desired point at which to create a parametric DMD. | ||
* @param[in] controls The matrices of controls from previous runs which we use to interpolate. |
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.
Non-uniform indents
6ef8179
to
3eceaa3
Compare
Adds parametric DMDc for heat conduction.
Example changing parameter in state vector:
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline --kappa 0.5 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline --kappa 0.75 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline --kappa 1.25 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline --kappa 1.5 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 --kappa 1 -online -predict -visit
Output:
Relative error of DMDc temperature (u) at t_final: 0.5 is 0.0025944158
Elapsed time for solving FOM: 1.006058e+00 second
Elapsed time for training DMDc: 7.535820e-01 second
Elapsed time for predicting DMDc: 3.593333e-03 second
Example changing parameter in control vector:
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline -amp-in 1 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline -amp-in 2 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline -amp-in 4 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -offline -amp-in 5 -rdim 6
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 -amp-in 3 -online -predict -visit
Output:
Relative error of DMDc temperature (u) at t_final: 0.5 is 0.0028885833
Elapsed time for solving FOM: 9.007740e-01 second
Elapsed time for training DMDc: 7.236116e-01 second
Elapsed time for predicting DMDc: 5.489291e-03 second
Some things I am suspicious of:
mpirun -np 8 parametric_dmdc_heat_conduction -s 1 --kappa 1 -amp-in 1000 -online -predict
the relative error for the final temperature is 0.013669751 despite the fact that
-amp-in 1000
is far outside our trained values for the inlet amplitude.matrixinterpolator.cpp
. I think other examples that call the matrix interpolator should still work with the new logic, since we are now just checking if dimensions match. I still need to verify this.