Skip to content

Use third order B.C. in MMS/advection/weno3 test case. - #1528

Merged
ZedThree merged 3 commits into
nextfrom
fix_advection_weno3_mms
Feb 1, 2019
Merged

ZedThree merged 3 commits into
nextfrom
fix_advection_weno3_mms

Conversation

@d7919

@d7919 d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member

With this change recover third order convergence

With this change recover third order convergence
@ZedThree

Copy link
Copy Markdown
Member

Fixes #1499

I think that just switching to dirichlet_o3 implies that dirichlet_o4 is probably broken

@johnomotani

johnomotani commented Jan 24, 2019

Copy link
Copy Markdown
Contributor

I think I tested dirichlet_o4 successfully in the test-twistshift-with-BoundaryOp-refactor branch, using the test in https://github.com/boutproject/BOUT-dev/blob/test-twistshift-with-BoundaryOp-refactor/tests/MMS/difops/runtest.py
That test used the BoundaryOp-refactor branch, but the stencils for dirichlet_o4 were the same (I have just now checked the CELL_CENTRE stencils). #1315 is not very nice, but @ZedThree's comment does suggest that a nicer version that can MMS both difops and boundary conditions would be nice to have...

@dschwoerer

Copy link
Copy Markdown
Contributor

I also tested dirichlet_nu_o4, and that gave the same result as dirichlet_o4 - thus I am also sceptical that dirichlet_o4 is actually broken ...

Could it be that dirichlet_o3 is broken, and thus gives 3rd order convergence? Assuming it introduces an sufficiently large error, that decreases with 3rd order, that would falsely give 3rd order convergence?

@d7919

d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member Author

I also tested dirichlet_nu_o4, and that gave the same result as dirichlet_o4 - thus I am also sceptical that dirichlet_o4 is actually broken ...

Could it be that dirichlet_o3 is broken, and thus gives 3rd order convergence? Assuming it introduces an sufficiently large error, that decreases with 3rd order, that would falsely give 3rd order convergence?

I think that's certainly possible.

@dschwoerer

Copy link
Copy Markdown
Contributor

Running it over a wider range, shows we are transitioning from 3.3 to ???, rather then having 3rd order convergence:

  16 |   2.074 s | ?
  32 |   3.835 s | 3.339857
  64 |  13.989 s | 3.359458
 128 |  24.719 s | 2.943565
 256 |  21.445 s | 2.194482
 512 |  95.321 s | 1.484496

@d7919

d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member Author

Running it over a wider range, shows we are transitioning from 3.3 to ???, rather then having 3rd order convergence:

  16 |   2.074 s | ?
  32 |   3.835 s | 3.339857
  64 |  13.989 s | 3.359458
 128 |  24.719 s | 2.943565
 256 |  21.445 s | 2.194482
 512 |  95.321 s | 1.484496

Ah, yes that looks bad. Have you tried this including the fix in #1529 ? I found this didn't make much difference at the 128 scale but it may help as we go beyond that.

@dschwoerer

Copy link
Copy Markdown
Contributor

Yes, I thought it wasn't but as that the derivatives are templated, switching to this branch didn't trigger a rebuild, so the patch was still included.

@d7919

d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member Author

It seems that if we use fourth order methods for the central methods in conjunction with this change to the boundaries we get at least third order convergence over an extended range.

@d7919

d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member Author

Switching to the fourth order boundaries the convergence order is reported as worse but the l2 error seems to be getting relatively small (~1.0e-5) and "bottoming out". If we reduce the size of the hyper-diffusion this bottoming out happens at a smaller scale (and we can recover third order if we're lucky -- it depends which grid points we choose to use).

@d7919

d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member Author

With third order BC the l2 error is smaller than with fourth order. The convergence order from 32 to 1024 points is >3 for every step (in powers of two), which I think is promising. This is true with the hyper-diffusion completely disabled and enabled, but the magnitude of the error is somewhat smaller with the hyper-diffusion disabled.

@d7919

d7919 commented Jan 24, 2019

Copy link
Copy Markdown
Member Author

For clarity this is without the change in #1529

Allows a bit more of the favourable range to count as a pass
@ZedThree
ZedThree merged commit b74472f into next Feb 1, 2019
@ZedThree
ZedThree deleted the fix_advection_weno3_mms branch February 1, 2019 16:57
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.

4 participants