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

compressible: add a get_external_sources method #290

Merged
merged 13 commits into from
Oct 21, 2024

Conversation

zingale
Copy link
Collaborator

@zingale zingale commented Oct 18, 2024

this will make it easier to do external sources problem-by-problem

For the conservative update, we now do a predictor-corrector, in the same fashion as Castro:

U^{n+1,*} = U^n - dt [div{F}]^{n+1/2} + dt S^n
S^{n+1} = S(U^{n+1,*})
U^{n+1} = U^{n+1,*} + dt/2 (S^{n+1} - S^n)

and we construct the gravity part of S^{n+1} to give us the implicit update.

@zingale zingale requested a review from zhichen3 October 18, 2024 15:01
@zingale zingale changed the title [WIP] compressible: add a get_external_sources method compressible: add a get_external_sources method Oct 18, 2024
((dens.v() + old_dens.v())*grav +
(ymom.v()**2 / dens.v() +
old_ymom.v()**2 / old_dens.v()) / myg.x2d.v()) - \
((ymom.v()**2 / dens.v() +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do the same for geometric source terms. One issue that bugged me before is how geometric source terms for xmom and ymom are coupled. So here I updated xmom first and used xmom for the ymom update, which is not "symmetric". with the predictor-corrector, we updaet them simultaneously i think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, we should. That is what I wanted to suggest to you. If you are okay with that, then I'll roll them into this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I've implemented this.


S[:, :, ivars.iener] = xmom_new * grav

S[:, :, ivars.ixmom] += U[:, :, ivars.iymom]**2 / (U[:, :, ivars.idens] * myg.x2d)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it not better to add geometric terms before we do xmom_new?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe. I think that they are both second order accurate. If we do geometry and then gravity, then the geometry momentum doesn't know about gravity and if we do gravity and then geometry, then the gravity doesn't know about geometry.

(ymom.v()**2 / dens.v() +
old_ymom.v()**2 / old_dens.v()) / myg.x2d.v()) - \
self.dt * (qx.ip(1, n=self.ivars.ip) - qx.v(n=self.ivars.ip)) / myg.Lx.v()
xmom.v()[:, :] += self.dt * (qx.ip(1, n=self.ivars.ip) -
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should be negative sign when we add the pressure term.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep. good catch. I fixed them both.

@zingale zingale merged commit dd2ec06 into python-hydro:main Oct 21, 2024
13 checks passed
@zingale zingale deleted the external_sources branch October 21, 2024 15:05
@zingale zingale restored the external_sources branch October 21, 2024 15:07
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.

2 participants