gridapapps / GridapGeosciences.jl

Gridap drivers for geoscience applications
MIT License
35 stars 3 forks source link

Rosenbrock solver for the SWEs #18

Closed amartinhuertas closed 3 years ago

amartinhuertas commented 3 years ago

Creating PR so that we can start discussing this work as needed ... WIP ...

davelee2804 commented 3 years ago

Here are the outstanding tasks that I'm aware of (there may be others)

codecov-commenter commented 3 years ago

Codecov Report

Merging #18 (1b8cee6) into master (e1439c5) will increase coverage by 6.32%. The diff coverage is 91.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   62.72%   69.05%   +6.32%     
==========================================
  Files          11       13       +2     
  Lines         440      559     +119     
==========================================
+ Hits          276      386     +110     
- Misses        164      173       +9     
Impacted Files Coverage Δ
src/DiagnosticTools.jl 58.33% <0.00%> (ø)
src/Helpers.jl 90.90% <90.90%> (ø)
src/ShallowWaterRosenbrock.jl 91.66% <91.66%> (ø)
src/ShallowWaterExplicit.jl 97.19% <100.00%> (+1.43%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e1439c5...1b8cee6. Read the comment docs.

davelee2804 commented 3 years ago

Hey @cbladwell , you've done some nice work here, looks like we're almost finished, well done! I'm just setting up to run your code now, but here are a few (minor) and one major comment in the interim:

cbladwell commented 3 years ago

I have update the multiplication by `Amat.

looks like this has been defined in a couple of places, might want to consolidate... https://github.com/gridapapps/GridapGeosciences.jl/blob/shallow_water_rosenbrock/src/ShallowWaterRosenbrock.jl#L10

Agreed. I haven't moved it yet but this could be an internal function _clone_fe_function in Operators.jl

davelee2804 commented 3 years ago

Thanks @cbladwell , I just fixed up a bug and made a minor optimisation and pushed the changes...

davelee2804 commented 3 years ago

Galewsky test case results look good (instability & conservation errors). Ready for revie vort_day6 w vort_cons

mass_cons ener_cons

amartinhuertas commented 3 years ago

@davelee2804 @cbladwell thanks for your work. I am happy with the current version. Once the tests pass we can accept the PR.