-
Notifications
You must be signed in to change notification settings - Fork 115
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
WIP: Taal #187
WIP: Taal #187
Conversation
By the way, I've marked this as "high-priority", since @gregorgassner categorized Taal in this way in our last meeting. |
Codecov Report
@@ Coverage Diff @@
## dev #187 +/- ##
==========================================
- Coverage 91.53% 89.24% -2.30%
==========================================
Files 36 52 +16
Lines 8037 9923 +1886
==========================================
+ Hits 7357 8856 +1499
- Misses 680 1067 +387
Continue to review full report at Codecov.
|
We now have more analysis stuff, but that requires SciML/OrdinaryDiffEq.jl#1272 for 2N low-storage methods. Memo: Set compat entry for dependencies on OrdinaryDiffEq accordingly once that PR is merged and a new version is released. |
I've now finished a first draft of the most important design decisions and most features are implemented in 2D. Thus, it's a good time to get some feedback!
@sloede @gregorgassner @andrewwinters5000 Could you please review this PR carefully? |
When everything looks okay to you, I would like to merge this into |
# TODO: Taal, decide where to define the entry points of our type hierarchy, | ||
# e.g. AbstractEquations, AbstractSemidiscretization etc. | ||
# Placing them here will allow us to make use of them for dispatch even for | ||
# other stuff defined very early in our include pipeline, e.g. | ||
# IndicatorLöhner(semi::AbstractSemidiscretization) |
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.
What are the downsides to doing this? I don't see any, except that I'd prefer to put those entry points into a separate file if it becomes more than ten lines or so, just to keep Trixi.jl
easy to read for someone new.
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.
It's okay for me to put these into a separate file. The only downside could be that someone would expect to find the definition of AbstractEquations
in equations.jl
instead of abstract_types.jl
. But that might not be a problem if we leave a comment in Trixi.jl
and use a descriptive name for the source file containing the abstract type definitions.
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.
Sounds good!
walkexpr(f, expr::Expr) = f(Expr(expr.head, (walkexpr(f, arg) for arg in expr.args)...)) | ||
walkexpr(f, x) = f(x) | ||
|
||
function mapexpr(expr; kwargs...) | ||
walkexpr(expr) do x | ||
if x isa Expr | ||
for (key,val) in kwargs | ||
if x.head === Symbol("=") && x.args[1] === Symbol(key) | ||
x.args[2] = :( $val ) | ||
# dump(x) | ||
end | ||
end | ||
end | ||
return x | ||
end | ||
end | ||
|
||
function test_trixi_include(parameters_file; l2=nothing, linf=nothing, |
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.
These methods should get at least some rudimentary high-level comment/docstring on what their purpose is and how they do it.
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'll do that.
# TODO: Taal, decide where to define the entry points of our type hierarchy, | ||
# e.g. AbstractEquations, AbstractSemidiscretization etc. | ||
# Placing them here will allow us to make use of them for dispatch even for | ||
# other stuff defined very early in our include pipeline, e.g. | ||
# IndicatorLöhner(semi::AbstractSemidiscretization) |
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.
Sounds good!
This implements a first prototype of Taal: Trixi as a library. There is still a lot of work to do before we reach feature parity with the previous versions, but we can run a simple example such as
examples/2d/parameters.toml
→examples/2d/parameters.jl
. I wanted to push this now so that you can get an impression of the look & feel and we can discuss how to proceed.TODO
Trixi.include
with the ability to modify stuff in the REPL via keyword argumentspolydeg
andcfl
explicit parameters in order to be able to modify them via keyword arguments inTrixi.include
abstract_types.jl
to enable dispatch on them throughout Trixi.jlconvtest
Vector
-likeu
from DiffEq.jl and "our" solution arrayu
and documentation/explanation ofwrap_array
return
, update the dev docs accordinglyelixir_eqs_specification.jl
in general:elixir_advection_basic.jl
instead ofparameters.jl
# TODO: Taal
Connections to issues
Xref #185 #177 #161 #150 #139 #136 #83 #70 #19 #10
Closes #173; closes #73