-
Notifications
You must be signed in to change notification settings - Fork 43
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
Soret isothermal wall fix #391
Soret isothermal wall fix #391
Conversation
Is it possible to check in the test case? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasHowarth thanks for figuring this out and adding the test case. After this PR gets merged, I can submit a separate PR to have the case run in testing suite including a check on species balance.
I've been trying out the test case. I got rid of dt_shrink to take larger timesteps and make errors more visible. This fix does drastically reduce, but not quite eliminate, species imbalance errors tracked through temporals/tempSpecies
. The remaining imbalance is small enough that I'm not very concerned, but I also don't get why conservation isn't quite exact (it doesn't seem to improve by tightening the diffusion solve tolerances).
A couple comments:
- Can the same fix be applied to the EB version of diffuse_scalar? That wouldn't eliminate the issue of isothermal EBs, but would at least allow EB simulations with isothermal domain boundaries and Soret.
- It appears that the updated BCs are not applied when diffusion fluxes are computed to compute divu. This should not affect species conservation and with the chi correction on divu, it should not affect the results in the limit of converged SDC iterations.
Without the Wbar correction, I have also managed to eliminate the conservation error (Up to 10^-23). However, iterating around the wbar fluxes seems to blow up the gradients, and I'm not entirely sure why. I'll keep working on it, but I've also pushed the latest version so you can take a look if you like. |
Nice progress a @ThomasHowarth. This has turned out to be much more involved than expected! If a solution doesn't jump right out at you for the exploding wbar iterations, do you think we can merge this without the wbar iterations (or maybe hardcoding to 1 "iteration") and then have a separate PR for the final correction? There's a lot here that I don't want to risk falling out of date and even without the last bit it's more correct than what's in there now. |
I think perhaps the best course is to just do the Soret correction for the conservation, disable the use_wbar for consistency (in these cases) and leave a warning/todo note. If this seems reasonable I can tidy things up and leave it like this for now? As you say, at least it will work, if not perfect yet. |
So, I've done some more thinking about why things blow up. Since we have to do some portion of the solve explicitly rather than lagged, I think we revert back to the point I made in the discussion, and the Y coefficient of the Wbar term becomes a problem. As I say, I can either disable the Wbar corrections in this case and everything works, otherwise this is going to become even more complicated... |
@baperry2 I have resorted to disabling wbar for this particular case - the boundary condition becomes very complicated otherwise. Some of the machinery is still left (e.g. filling boundaries for wbar and computing gradients of wbar using gradients of Y), but it should be bypassed. I'm leaving it there in case someone wants to try and figure this out down the line - I can also remove it. This way the solver is both conservative and consistent, even if the diffusion model isn't perfect. Given the number of commits, and switch arounds, it is probably best to squash this merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plan sounds good. Agree on leaving the wbar code for isothermal Soret in in case we want to revisit. Thanks again for doing this.
Good point on squashing and merging. We actually have GitHub rules set to require squashing and merging for all PRs now!
@jrood-nrel any idea why the This doesn't touch Docs, so i'm going to merge, but we should figure that out since it appears to be affecting all PRs |
It requires certain files to have changed. I assume that's it. PeleLMeX/.github/workflows/docs.yml Lines 7 to 9 in b3d27d4
|
I think this is fine, I'll just remove it from the list of "required" checks then, otherwise it will show up as "expected" and prevent auto-merging for cases where it does not run. |
This PR addresses #389 . Happy to discuss any of the implementation, and let me know if you can see spots for improvement/clarity. Currently, I've not implemented the necessary changes required for EB.