-
Notifications
You must be signed in to change notification settings - Fork 114
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
Jo/support 1d #188
Jo/support 1d #188
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.
@JuliaOd I did a first round of reviews, and most of your changes look great! I haven't reviewed dg.jl and dg/amr.jl yet, since they were changed in the current master and might need to be redone (especially when the new boundary condition stuff is merged, which we'll want to have in 1D as well). However, this looks to be on a very good track!
Note that I left a lot of comments with suggestions; if you approve of them, you can apply them automatically (see also https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request).
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #188 +/- ##
==========================================
- Coverage 91.60% 91.53% -0.07%
==========================================
Files 31 36 +5
Lines 6958 8037 +1079
==========================================
+ Hits 6374 7357 +983
- Misses 584 680 +96
Continue to review full report at Codecov.
|
@JuliaOd You should add your name to AUTHORS.md |
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.
Looks good to me (except some minor comments). Great work 👍
One feature I would like to have in 1D concerns visualization: Instead of using trixi2txt
from utils/trixi2txt.jl
to write a .txt
file and read that again in Julia to plot something, could we just have a function returning the respective x
and u
arrays directly? Then, users can just create their own plots in Julia directly without having to deal with another set of files.
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Remove comments Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]> Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
This sounds like a good idea. I'll open an issue for it. Update: #199 |
First attempt for a 1D solver.