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

Additional SGS Models #802

Merged
merged 18 commits into from
Jun 25, 2024
Merged

Conversation

JhonCordova
Copy link
Contributor

The WALE and Vreman models were integrated into the PeleC based on the rationale behind the original implementation of the Smagorinsky model available in this solver. Note that I have tried to encapsulate part of the code used in the 'getSmagorinskyLESTerm' method. Therefore, if there are any alternative suggestions, I am open to new ideas.

@marchdf marchdf requested review from baperry2 and marchdf May 28, 2024 15:26
@marchdf
Copy link
Contributor

marchdf commented May 28, 2024

You can remove all the files in refdata most likely. We don't need extra copies of those.

Source/PeleC.cpp Outdated Show resolved Hide resolved
@marchdf
Copy link
Contributor

marchdf commented May 28, 2024

Thank you for doing this! We will take a look at it this week. If you want to get the CI to start passing, you will need to do some formatting using clang-format.

@JhonCordova
Copy link
Contributor Author

Thank you for the feedback. I will address the comments shortly.

@baperry2
Copy link
Contributor

Thanks for doing this, and sorry it's taken a while for us to review.

It looks like the HIT_LES case is just a copy of the HIT case, except the input file. Is that right? If so, you can just put a hit-les.inp input file in the HIT case to avoid duplication of all those files. If you do that, I'll add it to the testing suite to ensure that the new LES models do not break in the future.

Have you run some tests with the WALE/Vremen models, particularly in comparison to Smagorinsky? If so, posting plots of the results here would be helpful.

Source/LES.H Show resolved Hide resolved
Source/LES.cpp Outdated Show resolved Hide resolved
Source/LES.cpp Fixed Show fixed Hide fixed
@JhonCordova
Copy link
Contributor Author

Thanks for doing this, and sorry it's taken a while for us to review.

It looks like the HIT_LES case is just a copy of the HIT case, except the input file. Is that right? If so, you can just put a hit-les.inp input file in the HIT case to avoid duplication of all those files. If you do that, I'll add it to the testing suite to ensure that the new LES models do not break in the future.

Have you run some tests with the WALE/Vremen models, particularly in comparison to Smagorinsky? If so, posting plots of the results here would be helpful.

Thanks for the feedback!

1. HIT_LES Case Update:
I've removed the HIT_LES case and included the hit-les.inp input file in the HIT case. This should streamline the process and avoid any unnecessary duplication of files.

2. SGS Models Comparison:
I have conducted tests comparing the WALE and Vreman models to the Smagorinsky model. The results and detailed analysis are presented in my publication, which you can access here.

Let me know if there's anything else you need or if further testing is required.

Source/PeleC.H Outdated Show resolved Hide resolved
@marchdf
Copy link
Contributor

marchdf commented Jun 21, 2024

@JhonCordova thanks for bearing with us as we wrangle this in. We really appreciate you contributing to the code and making it better. Good work!

Copy link
Contributor

@baperry2 baperry2 left a comment

Choose a reason for hiding this comment

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

@JhonCordova I have added the hit-les case with WALE to run in the CI tests. I also took the liberty of making some changes to resolve warnings, so I think all tests should pass now. Thanks for posting the link to your paper, that will be a good reference for anyone using the newly implemented models.

@baperry2 baperry2 merged commit 15854e8 into AMReX-Combustion:development Jun 25, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants