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

ESFR Split Formulation #111

Merged

Conversation

AlexanderCicchino
Copy link
Collaborator

@AlexanderCicchino AlexanderCicchino commented Feb 25, 2022

This pull request contains the ESFR split form. The changes in this pull request are as follows:

  1. The DG Strong form explicit introduced: constructing metric terms on the fly along with the assembly of metric dependent operators on the fly. It enables uncollocated split-forms with respect to the curvilinear gradient/divergence operator assembled based on whether the gradient, divergence or hybrid of them both are to be used. Additionally, the operators class was used throughout to clearly distinguish between what is truly being interpolated on what, and all the necessary projections intertwined. A clear example of this would be on the surface terms with interpolating volume fluxes from volume cubature nodes to the facet.
  2. Consistently clear explicit references to what is physical and what is reference is performed throughout DG Strong. This is critical for provably stable curvilinear tests. An example being the use of a physical normal for the surface numerical flux, but a reference normal for the interpolation of the reference volume flux interpolated to the facet.
  3. In DG, the mass matrix now has ESFR capabilities through the operators class. Additionally, for curvilinear elements, the mass matrix has the option to use a weight-adjusted low-storage implementation for both DG and ESFR cases. All ESFR mass matrices are verified through the operators volume operators test.
  4. The metric terms assembled via the operators class are provably free-stream preserving via GCL tests. Default is the conservative curl for, it also can use the invariant curl form by passing the paramter "use_invariant_curl_form." Both forms have unit tests for GCL in both the volume and surface, along with ensuring the surface metrics are conforming, and surface parametric GCL tests.
  5. ODE explicit solver has Runge-Kutta 4 added to it. Also, ODE solver base has modifications for both orders of accuracy versus energy tests. The modification specifically is an if statement checking whether an energy test is used. This will be an issue to raise after the pull request to add the energy and conservation check within the ode class.
  6. Unsteady source terms were added, with DG tracking the current time. For a future issue, the unsteady source term should be introduced in the manufactured solution class with time dependence. In manufactured solution, there is an "Alex function": this was used from the lab group meeting where I showed how to use the manufactured solution. I will use this as a base reference for the unsteady time dependent source in the future issue.
  7. Surface splitting functions were introduced in the physics classes.
  8. Central numerical flux was added for stability tests for linear advection on curvilinear grids. Entropy conserving numerical flux added currently only for Burgers' equation. (Both these fluxes conserve entropy for their respective test cases)
  9. Conservation and energy tests implemented for general FR problems in the unsteady tests. Example: src/testing/burgers_stability.cpp
  10. Burgers, curvilinear linear advection, and convection-diffusion strong form unsteady tests added. Also, TGV test cleaned up. In curvilinear advection unsteady, there are 3 mesh options, currently with 2 commented out through #if 0 #endif statements. The meshes are completely unsymmetric, skew-symmetric, and symmetric. This is an issue to be corrected after the pull request by allowing the meshes to called through the control file.
  11. Operator class allows flexibility for different polynomial and mesh orders. This is important because for freestream preservation, the volume mesh must be order p while the normals can be p+1 (superparametric). This is verified through the GCL and GCL superparamteric normals tests.
  12. Auxiliary variable introduced for diffusion type problems. Also, it is verified with its own unit tests.
  13. For all physics files, the change was adding time dependence to the source term and surface splitting. Currently, Euler doesn't have surface splitting and that's to be added in future issue.
  14. Entropy conserving numerical fluxes for all physics types.

Operators:

  1. each operator is it's own class derived from SumFactorizedOperators which is derived from an OperatorsBase. Sum factorization is only used in the APPLICATION of a matrix vector product. For example, when assembling the mass matrix to store in DG, sum factorization is NOT used because it does not apply the matrix. What is used is building the 1D object once before the cell loop and using that to build the mass matrix.
  2. the operators with sum-factorization is implemented through DGStrong giving a dramatically large improvement in computational cost. In the future, we will want to deprecate the use of fe_values and use the operators class.

Please see the attached document on my suggestion for the review:

ESFR_Pull_Request.pdf

Please see attached document below for an update on April 20, 2022.

ESFR_PR_Status_Apr20_2022.pdf

Additional contributions:

Alexander added 3 commits February 25, 2022 16:17
…akes use of operators in DG strong. Provides weight-adjusted curvilinear Mass matrices. Includes free-stream preserving metric terms.
@dougshidong dougshidong marked this pull request as draft February 25, 2022 23:11
@dougshidong
Copy link
Owner

83k lines. Some file shouldn't be there. Also, what happened to previous PR?

@dougshidong
Copy link
Owner

Also, can you give a summary of the contributions?
For example:
Those 1600 lines of code are described by Donovan: #85
Those 1100 lines of code are described by Julien: #87

This would give a sense of why some files have been changed, especially when 122 files are touched.

@AlexanderCicchino
Copy link
Collaborator Author

Size: yes I accidentally pushed the mesh, I have now removed it.
Previous PR: Since my last commits from the last PR were intertwined, when I went to rebase, it was full with merge conflicts and became quite complicated. Thus, I branched off the last version of your master and manually copied and pasted all the changes to apply a single commit. The 2 "merge" commits and the removing the mesh commit will be squashed in a rebase tomorrow.
Yes, I agree, I will give a summary with relevance to each file. Thank you:)

@AlexanderCicchino
Copy link
Collaborator Author

Also, do to the copy and pasting manual changes, some of the files have just an extra empty line at the bottom. An example being the mesh adaptation file. This shouldn't be there and I will go through each file changed tomorrow and make the appropriate edits.

@AlexanderCicchino
Copy link
Collaborator Author

Sorry, I was making a summary for each file and their changes. I realized now that you meant in general:
The changes in this pull request are as follows:

  1. The DG Strong form explicit introduced: constructing metric terms on the fly along with the assembly of metric dependent operators on the fly. It enables uncollocated split-forms with respect to the curvilinear gradient/divergence operator assembled based on whether the gradient, divergence or hybrid of them both are to be used. Additionally, the operators class was used throughout to clearly distinguish between what is truly being interpolated on what, and all the necessary projections intertwined. A clear example of this would be on the surface terms with interpolating volume fluxes from volume cubature nodes to the facet.
  2. Consistently clear explicit references to what is physical and what is reference is performed throughout DG Strong. This is critical for provably stable curvilinear tests. An example being the use of a physical normal for the surface numerical flux, but a reference normal for the interpolation of the reference volume flux interpolated to the facet.
  3. In DG, the mass matrix now has ESFR capabilities through the operators class. Additionally, for curvilinear elements, the mass matrix has the option to use a weight-adjusted low-storage implementation for both DG and ESFR cases. All ESFR mass matrices are verified through the operators volume operators test.
  4. The metric terms assembled via the operators class are provably free-stream preserving via GCL tests. Currently, it uses the conservative curl form with the invariant curl form commented out through #if 0 #endif statements. This is an issue for the future to add a parameter to call which one to be used. I currently didn't put it in because the invariant form copied from DG Weak does not actually satisfy GCL. As we discussed previously, it passed the free-stream test by solving for a different pde in a sense.
  5. ODE explicit solver has Runge-Kutta 4 added to it. Also, ODE solver base has modifications for both orders of accuracy versus energy tests. The modification specifically is an if statement checking whether an energy test is used. This will be an issue to raise after the pull request to add the energy and conservation check within the ode class.
  6. Unsteady source terms were added, with DG tracking the current time. For a future issue, the unsteady source term should be introduced in the manufactured solution class with time dependence. In manufactured solution, there is an "Alex function": this was used from the lab group meeting where I showed how to use the manufactured solution. I will use this as a base reference for the unsteady time dependent source in the future issue.
  7. Surface splitting functions were introduced in the physics classes.
  8. Central numerical flux was added for stability tests for linear advection on curvilinear grids. Entropy conserving numerical flux added currently only for Burgers' equation. (Both these fluxes conserve entropy for their respective test cases)
  9. Conservation and energy tests implemented for general FR problems in the unsteady tests. Example: src/testing/burgers_stability.cpp
  10. Burgers, curvilinear linear advection, and convection-diffusion strong form unsteady tests added. Also, TGV test cleaned up. In curvilinear advection unsteady, there are 3 mesh options, currently with 2 commented out through #if 0 #endif statements. The meshes are completely unsymmetric, skew-symmetric, and symmetric. This is an issue to be corrected after the pull request by allowing the meshes to called through the control file.
  11. Operator class allows flexibility for different polynomial and mesh orders. This is important because for freestream preservation, the volume mesh must be order p while the normals can be p+1 (superparametric). This is verified through the GCL and GCL superparamteric normals tests.
  12. Auxiliary variable introduced for diffusion type problems. Also, it is verified with its own unit tests.
  13. Operators unit tests verify all functions in the operators class. Also, operators metric terms have a check to make sure the grid did not change order since initialization. This is critical for the NACA type tests where the grid is initialized with order p then a different grid is read in.
  14. For all physics files, the change was adding time dependence to the source term and surface splitting. Currently, Euler doesn't have surface splitting and that's to be added in future issue.
  15. Currently DG initiliazes operators at declaration. This is because I followed the previous version of high_order_grid. A future issue is to make it a shared_ptr and initialize it similarly to high_order_grid.
  16. Lastly, all the filed with 1 line changed are due to the copy and paste error as discussed in last comment and I am working on getting rid of that.

Please let me know if there is anything that I over looked, needs clarification on, or if you'd like to discuss any/all of the above changes. Thank you!:)

@dougshidong
Copy link
Owner

Nice. Let us know when it's ready for review. You can edit the main post to move that description above.

@jbrillon jbrillon changed the title Esfr split form Merge ESFR Split Formulation Feb 28, 2022
@AlexanderCicchino
Copy link
Collaborator Author

I think it's good for review!

@dougshidong dougshidong marked this pull request as ready for review March 15, 2022 17:49
src/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Owner

@dougshidong dougshidong left a comment

Choose a reason for hiding this comment

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

I skimmed over parts of the code, but I'll review the all the files over different days. I think the first steps to make it easier on the reviewers:

  1. Remove all uses of #if 0 for comments. It makes the commented code appear as actual code and there is no tell for someone to know whether that piece of code is important or not. Feel free to even delete unused code, or move it to a function that is not being called. In git, everything is stored in its history. If it's an important alternative, consider using a parameter that is part of the input.
  2. Remove the files that have this trivial added line. Makes the review much bigger and harder to track
  3. Make sure your indent is consistent.
  4. Review it yourself! Best way to see if your code is suitable is to put yourself in the chair of the reviewer. If code looks ugly or you have to refresh your own memory because it's not clear, the same will apply from the point of view of the reviewer.

src/parameters/parameters_manufactured_solution.cpp Outdated Show resolved Hide resolved
src/numerical_flux/convective_numerical_flux.hpp Outdated Show resolved Hide resolved
src/numerical_flux/CMakeLists.txt Outdated Show resolved Hide resolved
src/numerical_flux/central_numerical_flux.cpp Outdated Show resolved Hide resolved
src/numerical_flux/entropy_cons_numerical_flux.hpp Outdated Show resolved Hide resolved
src/numerical_flux/entropy_cons_numerical_flux.cpp Outdated Show resolved Hide resolved
src/numerical_flux/central_numerical_flux.cpp Outdated Show resolved Hide resolved
src/numerical_flux/entropy_cons_numerical_flux.cpp Outdated Show resolved Hide resolved
@AlexanderCicchino
Copy link
Collaborator Author

Sorry for delay, McGill IT services/EMF completely wiped/erased my lab computer while I was in Toronto so I need to reinstall ubuntu and setup everything before making changes...

@AlexanderCicchino
Copy link
Collaborator Author

ESFR_Pull_Request.pdf

@AlexanderCicchino
Copy link
Collaborator Author

I have made the above necessary changes except the safeguard for the entropy numerical flux for Burgers because then I would need the numerical flux, or physics to store an AllParameters. In the future, there will be other flux functions in the entropy conserving num flux so I didn't make the change. Please let me know if that's ok, otherwise I think it is ready to continue the review. Lastly, after deleting the extra line, some files have this weird "no newline at end of file" git diff: from what I found online this is from the texteditor and I can't seem to find a way to fix it in vim.

@dougshidong
Copy link
Owner

Thanks! It's starting to look much better. I'll spend some time on this review in the upcoming days.

src/dg/weak_dg.cpp Outdated Show resolved Hide resolved
@jbrillon
Copy link
Collaborator

Attached are the ctest results, everything that is expected to pass is passing
ctest_20230223.txt

@cpethrick
Copy link
Collaborator

Approving as ctest is passing and issues have been opened for the remaining items. Good work, everyone!

@jbrillon
Copy link
Collaborator

jbrillon commented Feb 11, 2023

I skimmed over parts of the code, but I'll review the all the files over different days. I think the first steps to make it easier on the reviewers:

1. Remove all uses of #if 0 for comments. It makes the commented code appear as actual code and there is no tell for someone to know whether that piece of code is important or not. Feel free to even delete unused code, or move it to a function that is not being called. In git, everything is stored in its history. If it's an important alternative, consider using a parameter that is part of the input.

2. Remove the files that have this trivial added line. Makes the review much bigger and harder to track

3. Make sure your indent is consistent.

4. Review it yourself! Best way to see if your code is suitable is to put yourself in the chair of the reviewer. If code looks ugly or you have to refresh your own memory because it's not clear, the same will apply from the point of view of the reviewer.

Changing your review that requested changes to a review comment since the PR has addressed these changes over the last year of review. @dougshidong

@jbrillon jbrillon dismissed dougshidong’s stale review February 11, 2023 02:41

Comments have been addressed

Copy link
Collaborator

@jbrillon jbrillon left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants