Skip to content

Conversation

baperry2
Copy link
Contributor

@baperry2 baperry2 commented Aug 4, 2025

In simple transport coefficient, the transport coefficient for species i is roughly proportional to $\sum Y_{j \neq i}/\sum X_{j \neq i}$. This leads to zero divided by zero numerical issues for Yi = Xi= 1. These are sidestepped by adding an epsilon 1e-15 quantity to all mass fractions just for purposes of the diffusion coefficient calculation to ensure no mass fraction is ever exactly unity in these calculations.

This leads to numerical precision issues when we compute $\sum Y_{j \neq i}$ as $1 - Y_i$. Summing a bunch of quantities of O(1e-15) retains full floating point precision for sum, and therefore full floating point precision for the resulting diffusion coefficient. However, taking the difference between two O(1) quantities results in an O(1e-15) quantity with only O(1e-1) precision, and therefore results in a diffusion coefficient that can have O(1e-1) error, as seen in the EB-InflowBC test here: AMReX-Combustion/PeleC#971.

The proposed fix is to revert to computing $\sum Y_{j \neq i}$ instead of computing $1 - Y_i$, which unwinds part of the performance benefit from #595.

@baperry2 baperry2 requested a review from ThomasHowarth August 4, 2025 23:47
Ddiag[j] += Xloc[i] * dbintemp;
}
}
for (int i = 0; i < NUM_SPECIES; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could merge this for loop into the previous one, to avoid doing an extra N^2 loop, but also remove the if statement, i.e.

  1. we add term1=0 at the start of the loop
  2. in the first nested loop (j < i) we add term1 += yloc[j]
  3. Add another loop j=i+1, j < NUM_SPECIES which just adds the remaining part of term1
  4. Construct Ddiag in the i loop.

@ThomasHowarth
Copy link
Contributor

I had issues with pure species even before we added this change. I've been running pure hydrogen jets, and the diffusion solver was failing. I gradually increased the inflow mass fraction, and it was fine up to 99.9% hydrogen, but it would fail as soon as it hit 100%. Running with a fixed Lewis number ran fine, so I thought it was related to the mixture averaged formulation for pure species.

Copy link
Contributor

@ThomasHowarth ThomasHowarth left a comment

Choose a reason for hiding this comment

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

Also, the same changes need to be made for the equivalent SRK function

@baperry2
Copy link
Contributor Author

baperry2 commented Aug 11, 2025

@ThomasHowarth Good call on also making the changes for SRK.

Unless I'm missing something your proposed changes to the loop structure would result in the same number of FLOPS and I don't think we can move the full Ddiag within the first i loop so I did not implement that.

One potential issue with the trace stuff for pure component diffusion is that it permanently modifies the input Yloc after the transport function is called, which is unexpected behavior. I const protected Yloc in this function and created a new temporary array for the modified version. I don't think this should change much in LMeX, which creates its own temporary arrays to store Y (after computing it from rhoY).

In general the whole process here when applied to pure components makes me a bit uneasy because we're essentially dividing by zero with a numerical hack that makes it sort of work. Not sure what a better approach would be though.

@baperry2
Copy link
Contributor Author

baperry2 commented Sep 2, 2025

I'm going to merge this so we can get PelePhysics updates in PeleC going again. @ThomasHowarth we can revisit your suggestion on rearranging the loops later if you'd like

@baperry2 baperry2 merged commit 52fc1de into AMReX-Combustion:development Sep 2, 2025
13 checks passed
@ThomasHowarth
Copy link
Contributor

Sure thing @baperry2 - I opened a pull request into your branch a while ago with my suggestions if you want to take a look

@baperry2
Copy link
Contributor Author

baperry2 commented Sep 3, 2025

Sorry, somehow I completely missed that PR. I'll take a look.

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