-
Notifications
You must be signed in to change notification settings - Fork 63
Add 1D and 2D Wave Equation Examples using Mimetic Methods #57
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
Conversation
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 only reviewed examples/cpp/wave1D_case2.cpp. My comments are also applicable to the other cpp files you are adding in this PR.
Let me know if you want me to review the other files.
Thank you.
examples/cpp/Makefile
Outdated
|
|
||
| clean: | ||
| rm -f transport1D schrodinger1D parabolic1D elliptic1D elliptic2D convection_diffusion RK2 | ||
| rm -f transport1D schrodinger1D parabolic1D elliptic1D elliptic2D convection_diffusion RK2 wave1d wave1D_case2 wave2d wave2D_case2 |
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.
Should you start splitting these into multiple lines? I suggest:
all: transport1D \
schrodinger1D \
elliptic1D \
elliptic2D \
parabolic1D \
convection_diffusion \
RK2 \
wave1d \
wave1D_case2 \
wave2d \
wave2D_case2
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 have tried to stick to the structure but will modify the makefile exactly as you wrote. Thanks a lot.
examples/cpp/wave1D_case2.cpp
Outdated
|
|
||
| int main() { | ||
| // Parameters | ||
| const int k = 4; // Order of accuracy (spatial) |
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.
Should you change to constexpr here for all these constants? You know their values at compile time.
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 also think the readability of this code will improve if you adhere to a naming convention for constants. For instance, see: https://google.github.io/styleguide/cppguide.html#Constant_Names
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 suggestion. I used the same naming as the matlab file. However, sure, Why not. I will name them better
examples/cpp/wave1D_case2.cpp
Outdated
| } | ||
|
|
||
| // Save data for plotting | ||
| std::ofstream outfile("solution_" + std::to_string(step) + ".dat"); |
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 can use std::stringstream here for faster string-building (vs std::string).
std::stringstream filename;
filename << "solution_" << step << ".dat";
std::ofstream outfile(filename.str());```
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 for the suggestion. Using std::stringstream is indeed more efficient
|
Would you please review the new version of the wave1d_case2? I have tried to apply all your suggestions. Please let me know if I need to do anything else. Thanks a ton. |
It looks great, other than the two minor things I mentioned. Thanks for the changes! |
This information looks great! Would it be possible to have it on a readme file? |
|
I updated the README file and modified the wave2d code as well. Please review wave2d, wave1d, and wave1d_case2. Let me know if you have any questions. If everything looks good, I will proceed with updating wave2d_case2 as well. |
|
This is the content for CMakeLists.txt if it helps for code reproducibility. |
|
This still does not compile. If the cmake file needs to be edited, then it should be included in this PR. |
|
I updated the cmakelists and makefile. Please try to run it again with all the packages required. I proposed some new packages to be installed in the PR. |
|
New error on using cmake. |
|
Please try it again. The file name was wave1D_case2.cpp in my repo. It should have been wave1d_case2.cpp. I modified the names, and it should work now. |
|
Hi @ArshiaIlaty, Please try working with the CMake file I have mentioned below. Replace your Cmake file with this file as I have restructured it and avoided redundancy. Hope it resolves your issue. Thanks! |
|
Hi @JananiPSrinivasan |
|
Hi @ArshiaIlaty, The other maintainers and I have been chatting about your examples. They are pretty cool with the interactive visualization. Surely, this is something the other examples do not do. We are worried about the additional requirements your examples put on the MOLE library. The library already has a set of dependencies, and most are nonnegotiable, having to do with the mathematics involved and solving of sparse systems, which the library is primarily designed to do. With that, we are very hesitant to add additional dependencies, namely the boost library, the gnuplot library, and the C++20 requirement. Boost is a great library but is unnecessary for the core functionality of the MOLE library. Same for gnuplot. Gnuplot ships standard with Linux and interacts natively with C++, but requiring the dev libraries also might be asking too much of the casual user. And finally, the C++20 requirement. We wrote most of the library to be as compatible with older software as possible. Some of the codes using MOLE are older, with their own even older dependencies, and forcing a requirement to use 20 may have unforeseen implications on these other applications. What I do think is that you should keep the final working examples. A good compromise may be to simplify your examples using gnuplot for the MOLE library, and then you have your own hosted interactive examples, and we could link to your code from the MOLE example or repositories, something like a comment in wave1d.cpp like "If you want a more interactive Wave1D, go to https://github.com/ArshiaIlaty/mole_Ash and check out the interactive version". I would like @aboada , @jcorbino, @cpaolini, @valeriabarra to chime in on this in case I am out of line. And I am sorry @ArshiaIlaty for taking so long to bring this up. |
|
Please, even though that is not in the comments of the MATLAB code, indicate in the top comments of your code which equation is being solved, its time and space domains and its initial and boundary conditions. |
|
Has there been any progress on this? If not, I will close the pull request. |
|
On 2/13, @jbrzensk suggested removing in your code any dependencies on gnuplot dev, C++ 20 and boost. Were you able to do that? |
|
I have been working on it and will push the code for your review. Thank you. |
|
@aboada @jbrzensk Hi, hope you are doing well. Please share your thoughts on the new version of these four examples. I guess the conflicts for the gitignore and Makefile should be solved on your end. Please let me know if I should modify it. Thank you, Jared, for your comments, and thank you, Angel, for reviewing my code and guidance. |
|
Which version of the C++ compiler does your code requires? |
No need for version 20. I utilized 14 in the Makefile |
Thanks, let me know when your PR is ready for review. |
It is ready to review. I made the visualization cleaner. Please let me know your thoughts on the conflict of the .gitignore and Makefile. I assume I can delete my version or if you want I can resolve the conflict. Thank you |
|
@aboada: Is it required that the code is in C++ 11 or earlier? Ash's code seems to be C++ 14. |
Don't think we have established that dependency. > Ash's code compiles without errors on my fresh Ubuntu installation (using the From what I can see, my build is configured to use C++14, see: |
Even if there is a required dependency, I assume my code runs with version 11 as well. I have not tried to run it with 11, but I can give it a shot. |
jbrzensk
left a comment
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.
This is nice. I like the plotting, so much simpler!
You can remove a bunch of files, all that is needed is the wave1d, wave1d_case2, wave2d, wave2d_case2.
Most of my comments revolve around the casting MOLE operators to sparse matrices. All MOLE operators are sparse matrices by design. And there is a fix for the Interpolator error that shows up if you don't cast to sparse.
Please edit the src/cpp/operators.h file and add that to the pull request.
|
@jbrzensk Hi Jared, Thanks for reveiwing the code. I have tried to address all of your comments. Please recheck and let me know if there is something else I need to do. |
|
@jbrzensk Hi Jared, I did have a lot of conflicts and needed to rebase and so on. I think the final version of the codes and operators.h have been pushed. Please let me know if I should do anything else. |
|
@jbrzensk Thank you Jared for all of your guidance. |




1. wave1d.cpp: Implements the 1D wave equation solver using:
2. wave2d.cpp: Implements the 2D wave equation solver featuring:
Key Features:
Dependencies
Installation on different platforms:
macOS (using Homebrew):
Ubuntu/Debian:
PR Structure
examples/cpp/
├── wave1d.cpp # First case of 1D wave equation
├── wave1d_case2.cpp # Second case of 1D wave equation
├── wave2d.cpp # First case of 2D wave equation
├── wave2d_case2.cpp # Second case of 2D wave equation
└── Makefile # Build system for examples
Testing