Skip to content
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

Add ShallowWaterEquationsWetDry1D (2) #15

Closed
wants to merge 2 commits into from

Conversation

patrickersing
Copy link
Contributor

This PR adds the new equations ShallowWaterEquationsWetDry1D together with some tests.

The PR introduces only a first version of the equations that is supposed to work without any changes to Trixi.jl. For this reason some features that require changes in Trixi.jl are not moved yet. The main idea is to have a first working PR to test and discuss how to do the separation from Trixi.jl.

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (4ca97ca) 100.00% compared to head (99368cc) 88.73%.

Files Patch % Lines
src/equations/shallow_water_wet_dry_1d.jl 88.73% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              main      #15       +/-   ##
============================================
- Coverage   100.00%   88.73%   -11.27%     
============================================
  Files            1        1               
  Lines            3       71       +68     
============================================
+ Hits             3       63       +60     
- Misses           0        8        +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sloede
Copy link
Member

sloede commented Jan 12, 2024

Ci seems to work correctly 🥳

Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me, I only left a few smaller comments. however, I did not check the implementations in detail - I assume they are just copy-pasted from Trixi.jl and adapted for types (i.e., Trixi.AbstractEquations instead of AbstractEquations)?

Note that you should also modify .github/workflows/ci.yml to process the coverage data for the examples/ directory, similarly to how we do it in Trixi.jl

src/TrixiShallowWater.jl Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
test/test_trixi.jl Show resolved Hide resolved
@patrickersing
Copy link
Contributor Author

Yes, currently there are no additional features in this package and implementations were copied and adapted from Trixi.jl.

I have modified the ci.yml accordingly in aa21b7f.

As the problem with the status checks also popped up in this PR, I would resume to work on the original PR and close this one as soon as your comments are resolved.

@sloede
Copy link
Member

sloede commented Jan 15, 2024

As the problem with the status checks also popped up in this PR, I would resume to work on the original PR and close this one as soon as your comments are resolved.

Besides this comment,

Note that you should also modify .github/workflows/ci.yml to process the coverage data for the examples/ directory, similarly to how we do it in Trixi.jl

I think all is resolved.

@patrickersing
Copy link
Contributor Author

Note that you should also modify .github/workflows/ci.yml to process the coverage data for the examples/ directory, similarly to how we do it in Trixi.jl

With the fix in 00247c4 the modified ci.yml should now be working properly.

@sloede
Copy link
Member

sloede commented Jan 15, 2024

Note that you should also modify .github/workflows/ci.yml to process the coverage data for the examples/ directory, similarly to how we do it in Trixi.jl

With the fix in 00247c4 the modified ci.yml should now be working properly.

🎉

@patrickersing patrickersing deleted the swe_wetdry branch January 22, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants