-
Notifications
You must be signed in to change notification settings - Fork 1
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
Merging thesis_gemein_2022
into TrixiAtmo.jl
#2
Conversation
This is a good start for TrixiAtmo.jl, and should be quite useful. I will have some time to look into the specifics more in the next week, but my first thought is that we should perhaps set up testing/CI (similarly to TrixiShallowWater.jl) before merging this to main. |
Pull Request Test Coverage Report for Build 9959365955Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9970231868Details
💛 - Coveralls |
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.
Minor fixes
(Minor typographical errors)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
===========================================
+ Coverage 19.34% 50.90% +31.55%
===========================================
Files 8 8
Lines 827 827
===========================================
+ Hits 160 421 +261
+ Misses 667 406 -261 ☔ View full report in Codecov by Sentry. |
At first I though that we could maybe move some parts to TrixiBase.jl, but then I realized that these macro e.g. explicitly include the |
Which test case was 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.
It looks good to me, although I am wondering if either of you (@Arpit-Babbar or @benegee) know the reason for the warnings copied below? My tests have always been very simple so I've never run into this.
WARNING: replacing module TrixiAtmoTestModule.
WARNING: could not import TestExamples2DMoistEuler.TRIXI_EXAMPLES_DIR into TrixiAtmoTestModule
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 think it's good to merge if we're ok with the warnings I mentioned above.
You define a module with the same name every time in TrixiAtmo.jl/test/test_trixiatmo.jl Line 28 in 1c7efd9
If you want to get rid of that warning, you could |
This PR started as an explanation to #1. Now, it adds all the elixirs with respective CI tests from Lucas' thesis.
TODO