Separation of TrixiShallowWater.jl #1809

patrickersing · 2024-01-19T12:55:36Z

This PR prepares the separation of the planned TrixiShallowWater.jl package and removes some shallow water specific features, that will move into the new package.

Features that move to the new package are:

Two-layer shallow water equations (both 1D/2D)
Wet/Dry functionality which includes:
- threshold_limiter, threshold_wet
- PositivityPreservingLimiterShallowWater
- IndicatorHennemannGassnerShallowWater
- All functions related to the hydrostatic_reconstruction_chen_noelle

The features including respective examples and tests, will be removed from Trixi.jl and be included in TrixiShallowWater.jl. There, the shallow water equations with wet/dry functionality will be provided by a new equation set ShallowWaterEquationsWetDry. The basic ShallowWaterEquations and ShallowWaterEquationsQuasi1D without Wet/Dry functionality will remain in Trixi.jl.

In order to do the separation, this PR should be prepared together with a companion PR that introduces these features in TrixiShallowWater.jl. For the separation process we should then first merge this PR and then add these features in TrixiShallowWater.jl.

github-actions · 2024-01-19T12:55:48Z

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

The PR has a single goal that is clear from the PR title and/or description.
All code changes represent a single set of modifications that logically belong together.
No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

The code can be understood easily.
Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
There are no redundancies that can be removed by simple modularization/refactoring.
There are no leftover debug statements or commented code sections.
The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

New functions and types are documented with a docstring or top-level comment.
Relevant publications are referenced in docstrings (see example for formatting).
Inline comments are used to document longer or unusual code sections.
Comments describe intent ("why?") and not just functionality ("what?").
If the PR introduces a significant change or new feature, it is documented in NEWS.md.

Testing

The PR passes all tests.
New or modified lines of code are covered by tests.
New or modified tests run in less then 10 seconds.

Performance

There are no type instabilities or memory allocations in performance-critical parts.
If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

The correctness of the code was verified using appropriate tests.
If new equations/methods are added, a convergence test has been run and the results
are posted in the PR.

Created with ❤️ by the Trixi.jl community.

codecov · 2024-01-19T13:41:41Z

Codecov Report

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

Comparison is base (c3c0986) 89.67% compared to head (e96453f) 73.26%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/equations/shallow_water_1d.jl	66.67%	1 Missing ⚠️
src/equations/shallow_water_quasi_1d.jl	66.67%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1809       +/-   ##
===========================================
- Coverage   89.67%   73.26%   -16.41%     
===========================================
  Files         458      438       -20     
  Lines       36400    35342     -1058     
===========================================
- Hits        32640    25891     -6749     
- Misses       3760     9451     +5691

Flag	Coverage Δ
unittests	`73.26% <77.78%> (-16.41%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

andrewwinters5000

Nice work @patrickersing ! This rips out anything marked for removal having to do with the wet/dry functionality and returns the ShallowWaterEquations struct to its original form without the different threshold values. Also, the computation of the well-balancedness error returns to its original strategy.

We need to coordinate this breaking change to Trixi with the others and then deal with reviews on the re-implementation of all this functionality into your PR in TrixiShallowWater.jl.

For the modifications to the ShallowWaterEquationsQuasi1D we need some input from @jlchan . Does the testing and/or functionality of this equation system required the wetting and drying capabilities?

sloede · 2024-01-31T06:41:56Z

We need to coordinate this breaking change to Trixi with the others

Coordinate yes, but only in the sense that once both PRs are ready, we announce "we are doing this now", merge the PRs and create a new breaking Trixi.jl release immediately.

sloede · 2024-01-31T06:43:56Z

Ah, I forgot about #1726. If you are more or less ready here, please bring up this amicable divorce at the next Trixi.jl Meeting!

andrewwinters5000 · 2024-01-31T06:59:57Z

We need to coordinate this breaking change to Trixi with the others

Coordinate yes, but only in the sense that once both PRs are ready, we announce "we are doing this now", merge the PRs and create a new breaking Trixi.jl release immediately.

Yes, this is what I meant. As a development team we need to all be on the same page and "push the button" so to say on this divorce. In doing so, we can merge any other breaking changes that are ready to go.

andrewwinters5000 · 2024-01-31T07:03:32Z

Ah, I forgot about #1726. If you are more or less ready here, please bring up this amicable divorce at the next Trixi.jl Meeting!

I will add this to the meeting agenda for next week. Also, I will do a review of the corresponding PR in TrixiShallowWater.jl to make sure everything is clean there. That way, I figure the process would be

Merge this PR to Trixi main
Release a new version of Trixi that is then available for TrixiShallowWater
Clean-up any comments or so in this PR
Merge that PR into TrixiShallowWater main
Live happily ever after :)

sloede · 2024-01-31T07:10:26Z

That way, I figure the process would be

1. Merge this PR to Trixi `main`

2. Release a new version of Trixi that is then available for TrixiShallowWater

3. Clean-up any comments or so in [this PR](https://github.com/trixi-framework/TrixiShallowWater.jl/pull/18)

4. Merge that PR into TrixiShallowWater `main`

5. Live happily ever after :)

Sounds good to me! It would also be great if you could maybe do the first review of this PR here. From a brief look it seems to include only technical changes at the moment. However, I think we should think about how to present the relationship between Trixi.jl and TrixiSW.jl in the Trixi.jl docs. For instance, I think it should be reflected in the feature list of the README, it needs to be properly documented in the NEWS, and we should also maybe write a short statement about the growing Trixi.jl ecosphere (either in the README or in an extra section).

andrewwinters5000 · 2024-01-31T11:28:21Z

However, I think we should think about how to present the relationship between Trixi.jl and TrixiSW.jl in the Trixi.jl docs.

That is true, a strategy for the docs of both packages is something that needs to be hammered out and discussed.

andrewwinters5000

LGTM! It would be good if @ranocha can have a last look before the merge.

sloede

LGTM, though I did only a sanity check and didn't, e.g., verify exactly what was removed. What's missing, however, is an appropriate NEWS.md item

ranocha

I agree with @sloede - we should get some NEWS.md entry for breaking changes when upgrading to the new Trixi.jl version.

src/Trixi.jl

patrickersing · 2024-02-07T12:24:47Z

I have added an entry to NEWS.md regarding the breaking changes from this PR. Let me know if anything is missing.

andrewwinters5000

LGTM! I like the additional unit tests and the NEWS item looks good.

NEWS.md

ranocha · 2024-02-13T18:32:18Z

I would like to fix the recent CI failures before releasing the breaking version.

andrewwinters5000 · 2024-02-13T19:24:30Z

I would like to fix the recent CI failures before releasing the breaking version.

Okay, that is reasonable.

Co-authored-by: Hendrik Ranocha <[email protected]>

patrickersing and others added 7 commits December 19, 2023 13:43

remove wet_dry functionality for SWE-1D

771cf96

remove wet_dry functionality for SWE-2D

8448ae6

remove twolayer equations

73022de

Merge branch 'trixi-framework:main' into TrixiSW-separation

f0693f0

remove limiters from swe_quasi_1d

252eaea

remove export of min_max_speed_chen_noelle

7e15737

merge main

6febd5b

patrickersing marked this pull request as ready for review January 19, 2024 14:01

patrickersing requested a review from andrewwinters5000 January 19, 2024 14:01

patrickersing mentioned this pull request Jan 23, 2024

Add 2D-SWE and move wet/dry functionality from Trixi.jl trixi-framework/TrixiShallowWater.jl#18

Merged

Merge branch 'main' into TrixiSW-separation

1303108

patrickersing mentioned this pull request Jan 24, 2024

Fix downstream tests trixi-framework/TrixiShallowWater.jl#19

Merged

DanielDoehring added the breaking label Jan 24, 2024

Merge branch 'main' into TrixiSW-separation

41b5de3

andrewwinters5000 reviewed Jan 30, 2024

View reviewed changes

andrewwinters5000 mentioned this pull request Jan 31, 2024

Release v0.7 #1726

Closed

8 tasks

andrewwinters5000 and others added 4 commits January 31, 2024 13:00

Merge branch 'main' into TrixiSW-separation

1d3e112

Merge remote-tracking branch 'origin/main' into TrixiSW-separation

09893bf

remove elixirs

43ab8dd

Merge branch 'main' into TrixiSW-separation

cabcf4f

andrewwinters5000 previously approved these changes Feb 6, 2024

View reviewed changes

sloede requested changes Feb 6, 2024

View reviewed changes

ranocha requested changes Feb 7, 2024

View reviewed changes

src/Trixi.jl Show resolved Hide resolved

update news.md

f7d76d6

patrickersing dismissed andrewwinters5000’s stale review via f7d76d6 February 7, 2024 12:20

patrickersing and others added 2 commits February 7, 2024 13:25

Merge branch 'main' into TrixiSW-separation

21ea4fb

add unit tests to increase coverage

f667f80

patrickersing requested review from andrewwinters5000, ranocha and sloede February 8, 2024 07:51

andrewwinters5000 previously approved these changes Feb 8, 2024

View reviewed changes

patrickersing mentioned this pull request Feb 13, 2024

Failing tests related to t8code #1845

Closed

ranocha reviewed Feb 13, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

NEWS.md Outdated Show resolved Hide resolved

NEWS.md Outdated Show resolved Hide resolved

patrickersing dismissed andrewwinters5000’s stale review via 273d7e7 February 14, 2024 08:08

patrickersing and others added 4 commits February 14, 2024 09:08

Change news.md according to code review

273d7e7

Co-authored-by: Hendrik Ranocha <[email protected]>

Merge branch 'main' into TrixiSW-separation

068b4d1

Merge branch 'main' into TrixiSW-separation

0453855

Merge branch 'main' into TrixiSW-separation

e96453f

ranocha approved these changes Feb 23, 2024

View reviewed changes

ranocha merged commit 62610bb into trixi-framework:main Feb 23, 2024
28 of 36 checks passed

patrickersing mentioned this pull request Mar 11, 2024

Move two-layer shallow water equations from Trixi.jl trixi-framework/TrixiShallowWater.jl#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separation of TrixiShallowWater.jl #1809

Separation of TrixiShallowWater.jl #1809

patrickersing commented Jan 19, 2024 •

edited

Loading

github-actions bot commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

andrewwinters5000 left a comment

sloede commented Jan 31, 2024

sloede commented Jan 31, 2024

andrewwinters5000 commented Jan 31, 2024

andrewwinters5000 commented Jan 31, 2024

sloede commented Jan 31, 2024

andrewwinters5000 commented Jan 31, 2024

andrewwinters5000 left a comment

sloede left a comment

ranocha left a comment

patrickersing commented Feb 7, 2024

andrewwinters5000 left a comment

ranocha commented Feb 13, 2024

andrewwinters5000 commented Feb 13, 2024

Separation of TrixiShallowWater.jl #1809

Separation of TrixiShallowWater.jl #1809

Conversation

patrickersing commented Jan 19, 2024 • edited Loading

github-actions bot commented Jan 19, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

andrewwinters5000 left a comment

Choose a reason for hiding this comment

sloede commented Jan 31, 2024

sloede commented Jan 31, 2024

andrewwinters5000 commented Jan 31, 2024

andrewwinters5000 commented Jan 31, 2024

sloede commented Jan 31, 2024

andrewwinters5000 commented Jan 31, 2024

andrewwinters5000 left a comment

Choose a reason for hiding this comment

sloede left a comment

Choose a reason for hiding this comment

ranocha left a comment

Choose a reason for hiding this comment

patrickersing commented Feb 7, 2024

andrewwinters5000 left a comment

Choose a reason for hiding this comment

ranocha commented Feb 13, 2024

andrewwinters5000 commented Feb 13, 2024

patrickersing commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 19, 2024 •

edited

Loading