WENO7 #638
Merged
WENO7 #638
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Excellent! Glad to have new features. I still need to do code review... |
Thanks for helping us review the code! Just to let you know, I still need to rebase to the newest commit, which I can't do right now. It's probably a dependency issue: On the newest commit, a fresh build works, but afterwards, if I modify anything in m_weno.fpp, even if it's just a comment, the compilation fails. It continues to fail even after I change it back. It only works again if I delete the build folder and build again from scratch. This issue wasn't present several commits ago. The compilation error is: |
|
Having a look... in the meantime I found this which follows from your |
I can't reproduce this on my computer. Added some new lines and comments and deleted a few lines in |
|
Thanks for helping to test it! Looks like the dependency issue is specific to my system... I'll look for the cause |
Sorry forgot to push the commit that fixed this for WENO7M |
|
It's unclear why WENO7 is incompatible with grid stretching? We use the stretching feature often. |
It should be compatible, but I haven't done it. |
|
Also, the build error is not specific to my machine. The same error appears on both Bridges2 and Delta for CPU compilation when I:
I've narrowed down the problematic commit to: but I'm not exactly sure what exactly causes the problem. Maybe @henryleberre you have some ideas? |
|
Here's the error message on Bridges2 (same on Delta): |
|
@ChrisZYJ what compiler? |
|
FYI we've seen some weird issues like this before. I don't think it is MFC specific, though it's hard to know for sure. I haven't seen it in weno, to my memory, but in maybe other source code spots. In each case I really don't remember how we rectified the issue, but we did fix it, I think usually by accident. I would consider merging with upstream so at least you're consistent with master. Maybe that will "accidentally" fix it. |
Thanks for the suggestions! I apologize for not being clear in my explanation - the error occurs on a fresh clone of MFC, unrelated to this PR (I probably should have opened a separate issue for it). The error wasn't present before commit 2d1de77 but has been consistent since then. Therefore, it's most likely a problem with MFC introduced in that specific commit. I'm not certain if it's limited to m_weno.fpp. I've chosen to work from an earlier commit so I can continue working on WENO7 while this issue gets fixed. Otherwise, I'd need to delete the build directory and rebuild every time I want to test a change. |
I'm using GNU v11.4.0 on Ubuntu. |
Regarding narrowing down the commit that seems potentially responsible, I only find these things possibly suspicious:
|
@ChrisZYJ I tried this on Bridges2 and cannot replicate the issue. |
|
Reminder to add WENO7 and new variants to the |
Sorry for the trouble! This is really weird - I'll ask someone from my group to try it too. It could be a really silly mistake on my end. |
Thanks for the reminder, and thanks for helping me add it for WENO5 variants last time! |
|
Jose kindly helped me test on Bridges2 and he saw the exact same thing - first build works, second build fails after changing m_weno.fpp, then deleting build directory and rebuilding works |
|
I tried building again on Bridges2 and Delta using 1 processor, and got the same thing again. Here's the complete log from cloning to getting the errors, with the build details omitted: |
|
I will try it again soon. |
Thank you! |
@henryleberre @ChrisZYJ I found a smaller version of the problem and can reproduce your case:
(I removed the So, something about including I was able to reproduce it on my MacBook. In the broken case, it seemingly still pre-processes [fypp] Build | syscheck, simulation, and post_process | Generic Build
Generating case.fpp.
Writing a (new) custom case.fpp file.
$ cmake --build /Users/spencer/Downloads/MFC/build/staging/7d9b728a37 --target syscheck --parallel 10 --config Release
[100%] Built target syscheck
$ cmake --install /Users/spencer/Downloads/MFC/build/staging/7d9b728a37
-- Install configuration: "Release"
-- Up-to-date: /Users/spencer/Downloads/MFC/build/install/7d9b728a37/bin/syscheck
Generating case.fpp.
Writing a (new) custom case.fpp file.
$ cmake --build /Users/spencer/Downloads/MFC/build/staging/98998883b5 --target simulation --parallel 10 --config Release
[ 1%] Preprocessing (Fypp) m_weno.fpp
[ 2%] Building Fortran object CMakeFiles/simulation.dir/fypp/simulation/m_weno.fpp.f90.o
/Users/spencer/Downloads/MFC/src/simulation/m_weno.fpp:926:35:
926 | real(kind(0d0)), dimension(startx:, starty:, startz:, 1:), intent(IN) :: v_rs_ws
| 1
Error: Expression at (1) must be of INTEGER type, found REAL
/Users/spencer/Downloads/MFC/src/simulation/m_weno.fpp:927:35:
[......]in the same way that it does in the working case: lawn-100-70-35-133: Downloads/MFC $ ./mfc.sh build -j 10 -t pre_process simulation
mfc: OK > (venv) Entered the Python 3.12.6 virtual environment (>= 3.8).
.=++*: -+*+=. | spencer@lawn-100-70-35-133.lawn.gatech.edu [Darwin]
:+ -*- == =* . | ---------------------------------------------------
:*+ == ++ .+- |
:*##-.....:*+ .#%+++=--+=:::. | --jobs 10
-=-++-======#=--**+++==+*++=::-:. | --mpi --no-gpu --no-debug --no-gcov --no-unified
.:++=----------====+*= ==..:%..... | --targets pre_process and simulation
.:-=++++===--==+=-+= +. := |
+#=::::::::=%=. -+: =+ *: | ----------------------------------------------------------
.*=-=*=.. :=+*+: -...-- | $ ./mfc.sh (build, run, test, clean, count, packer) --help
Build | syscheck, pre_process, and simulation | Generic Build
Generating case.fpp.
Writing a (new) custom case.fpp file.
$ cmake --build /Users/spencer/Downloads/MFC/build/staging/7d9b728a37 --target syscheck --parallel 10 --config Release
[100%] Built target syscheck
$ cmake --install /Users/spencer/Downloads/MFC/build/staging/7d9b728a37
-- Install configuration: "Release"
-- Up-to-date: /Users/spencer/Downloads/MFC/build/install/7d9b728a37/bin/syscheck
Generating case.fpp.
Writing a (new) custom case.fpp file.
$ cmake --build /Users/spencer/Downloads/MFC/build/staging/9a4af0a3bd --target pre_process --parallel 10 --config Release
[100%] Built target pre_process
$ cmake --install /Users/spencer/Downloads/MFC/build/staging/9a4af0a3bd
-- Install configuration: "Release"
-- Up-to-date: /Users/spencer/Downloads/MFC/build/install/9a4af0a3bd/bin/pre_process
Generating case.fpp.
Writing a (new) custom case.fpp file.
$ cmake --build /Users/spencer/Downloads/MFC/build/staging/98998883b5 --target simulation --parallel 10 --config Release
[ 1%] Preprocessing (Fypp) m_weno.fpp
[ 2%] Building Fortran object CMakeFiles/simulation.dir/fypp/simulation/m_weno.fpp.f90.o
[ 4%] Linking Fortran executable simulation
ld: warning: -ld_classic is deprecated and will be removed in a future release
ld: warning: -ld_classic is deprecated and will be removed in a future release
[100%] Built target simulationThis does strongly point to some sort of cmake issue introduced in #620 if the issue does not occur for commits before that. |
|
@sbryngelson That commit is not part of this PR (it is behind). I would suggest @ChrisZYJ rebase on upstream and then see if the issue persists. |
|
@henryleberre Maybe I should have opened a new issue to make it clearer - the problem is independent of this PR. All my errors are reproduced with fresh clones of MFC, and is consistent over the past few commits, so rebasing this PR probably wouldn't help. I chose to rebase this PR on a commit before #620, so I can continue working on WENO7 without the build issue. Sorry for the confusion. |
@ChrisZYJ Oh I see - now I'm not entirely sure who is confused! (probably me) - so this issue exists before PR 620? or you are not sure when exactly it appears (other than it exists now and for the past few commits)? |
|
Sorry for my bad explaination! Some of it got lost in the long converstaion too... Here's a (hopefully) better summary:
|
This makes sense and seems to point to #620 @henryleberre? |
|
Thanks @ChrisZYJ for the clarification. I should have read the thread more carefully (I only saw it when @sbryngelson tagged me). Also, sorry I missed you tagging me previously. Going through 2d1de77 again, I can't think of an obvious reason why this commit would introduce it but maybe one of the toolchain changes subtly allows for it. I can begin an investigation. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #638 +/- ##
=========================================
Coverage ? 54.54%
=========================================
Files ? 61
Lines ? 13800
Branches ? 1727
=========================================
Hits ? 7527
Misses ? 5817
Partials ? 456 ☔ View full report in Codecov by Sentry. |
|
@ChrisZYJ let me know the status of this PR when it makes sense to merge and I'll look carefully |
|
@sbryngelson Sorry for the wait! I've checked the performance and optimized for speed. It should be ready now. |
|
@ChrisZYJ I think the WENO smoothness indicators edit: There is also the generic formula here: https://www.wias-berlin.de/people/john/BETREUUNG/bachelor_rupp.pdf |
|
Thank you for the references - they are really helpful! While I've seen the first paper and computed some coefficients from its formulas, the simplified explicit forms in the second paper are particularly useful. Implementing these formulas into our code and validating the results will take quite some work. Since I'm currently focused on performing simulations that only require uniform grids, would it be alright to implement WENO7 for non-uniform grids in a separate PR? I can prioritize it if needed. Thanks! |
That sounds good! These papers came from a rather simple Scholar search, so there may be a better formula out there. I appreciate your clean PRs. I will review this soon, I have one in the queue before this. |
sbryngelson
approved these changes
Oct 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds WENO7-BS (the original WENO-JS but 7th order), WENO7-M, WENO7-Z, and TENO7.
Only works for uniform grid.
Type of change
Scope
How Has This Been Tested?
The left and right-travelling waves are nearly identical. The max-norm difference increases as the simulation proceeds, but is comparable to that of the original WENO5 schemes.
The loss of symmetry is expected for chaotic simulations using high order schemes (see Fu, et al., 2016 for examples). The symmetry of the WENO7 schemes has been checked (see 2-Way Shu-Osher Problem above).
Checklist
docs/)examples/that demonstrate my new feature performing as expected.They run to completion and demonstrate "interesting physics"
./mfc.sh formatbefore committing my codeIf your code changes any code source files (anything in
src/simulation)To make sure the code is performing as expected on GPU devices, I have:
nvtxranges so that they can be identified in profiles./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR nsys.txt./mfc.sh run XXXX --gpu -t simulation --omniperf, and have attached the output file and plain text results to this PR. (omni not attached as it took forever to complete. Instead, performence on Frontier is shown below)The performance comparison between WENO7 variants is as expected. In terms of speed: WENO-JS ≈ WENO-Z > WENO-M > TENO. TENO is more expensive than WENO-M due to the extra stencil required. WENO-Z has a variable power, which requires case-optimization to avoid significant performance penalties. Benchmarking shows that case-optimization for teno_CT has no impact on speed, so it has not been implemented.
What's surprising is that the WENO7 speed appears comparable to that of WENO5. This might be due to the use of hard-coded coefficients for uniform grids, which enables better compiler optimization. I can investigate this further in the future.