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

Linear boussinesq #491

Merged
merged 4 commits into from
Mar 27, 2024
Merged

Linear boussinesq #491

merged 4 commits into from
Mar 27, 2024

Conversation

jshipton
Copy link
Contributor

@jshipton jshipton commented Mar 22, 2024

Linear Boussinesq equation class and example

subject(prognostic(cs**2 * phi * div(u) * dx, 'p'), self.X),
linear_div_form), "divergence")
else:
# This enforces that div(u) = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the results look odd when running incompressible Boussinesq with RK4 but are fine with SIQN - this comment is about the forcing term which makes me suspicious...

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be related to issues @atb1995 saw when having a divergence term in the shallow-water equations? And in forcing.py we ensure that the incompressibility term is treated implicitly. So would we expect this to work with an explicit time stepper?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd assume it should work fine for small enough dt - the issue I was having was that the range of stable dt for the IMEX RK schemes was being limited by effectively treating a div(u) term explicitly. You could try using IMEX RK, with the div term labeled first explicitly, then implicitly and see if you see the same issues when it is explicit (but not when it is implicit). Or just use smaller dt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, it's not that - sorry, I should have been clearer. The results don't show instability - they're just wrong. And the reason is that this isn't really an evolution equation for the pressure. What we're trying to do is solve for the pressure that gives us a non-divergent velocity. When we use the SIQN time stepper, the incompressible term is implicit and, as the comment here says, we overwrite the pressure rather than updating it.

gusto/equations.py Outdated Show resolved Hide resolved
@jshipton
Copy link
Contributor Author

I think that the issue with the incompressible term should become an issue, since that's not what this PR was for. Would you like a test for the linear compressible case @tommbendall ?

@jshipton jshipton marked this pull request as ready for review March 26, 2024 19:15
@tommbendall
Copy link
Contributor

There are conflicts with this branch now so I'm afraid these will need resolving before this can go on.

I think PR is fine without a test given the example that you've added, and I agree that we should just have an issue to explain why this won't work for now with explicit timesteppers.

Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

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

Everything's passing so this is ready to merge!

@tommbendall tommbendall merged commit f8b8050 into main Mar 27, 2024
4 checks passed
@tommbendall tommbendall deleted the linear_boussinesq branch March 27, 2024 17:35
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