mutationpp / Mutationpp

The MUlticomponent Thermodynamic And Transport library for IONized gases in C++
GNU Lesser General Public License v3.0
106 stars 57 forks source link

Fix averageHeavyCollisionFreq in Python interface. #222

Closed mgoodson-cvd closed 10 months ago

mgoodson-cvd commented 1 year ago

Fix an issue with the averageHeavyCollisionFreq function in the Python interface. It was incorrectly returning the electronHeavyCollisionFreq, likely due to a copy-paste error.

See #221. Closes #221.

mgoodson-cvd commented 1 year ago

I should note that I didn't find this bug by trying to use the Python module; I noticed it in the source when I was grepping for the collision frequency. I've never tried the Python interface and did not test this PR. Looking at the logs above, the CI/CD failure seems unrelated to my changes.

codecov[bot] commented 1 year ago

Codecov Report

Attention: 71 lines in your changes are missing coverage. Please review.

Comparison is base (fcbd298) 71.48% compared to head (d223045) 71.66%.

Files Patch % Lines
src/transport/Transport.cpp 8.00% 69 Missing :warning:
src/apps/mppequil.cpp 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #222 +/- ## ========================================== + Coverage 71.48% 71.66% +0.18% ========================================== Files 135 135 Lines 9114 9091 -23 ========================================== Hits 6515 6515 + Misses 2599 2576 -23 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mgoodson-cvd commented 1 year ago

This Python test failure has done its job and more; it has uncovered a bug in the C++ code. The bug arises in the computation of the mean free path for mixtures that do not contain electrons. The mean free path is used in the computation of the average heavy collision frequency. The computation of the mean free path attempts to pull from Q11ee which is undefined for mixtures that do not contain electrons. The other routines that use Q11ee in this file have an initial check on if (i < m_thermo.hasElectrons()); the same will be needed here to adjust the behavior appropriately. I'll open a separate issue.

mgoodson-cvd commented 1 year ago

@jbscoggi The ubuntu-18.04 builds are still showing as required checks even after removing them from the workflow/build.yml configuration file. Any idea on how to remove these checks? I've just started looking into it.

rdbisme commented 1 year ago

@jbscoggi The ubuntu-18.04 builds are still showing as required checks even after removing them from the workflow/build.yml configuration file. Any idea on how to remove these checks? I've just started looking into it.

I removed the checks.

mgoodson-cvd commented 10 months ago

@jbscoggi I've re-worked the mean free path computation to a) avoid using unallocated arrays when electrons aren't present, and b) to use the updated flattened upper-diagonal array form of Q11. I've also corrected what I believe to be a minor bug in the electron portion of the computation which was neglecting the duplicate off-diagonal. I've checked a couple values for the mean free path, and they are definitely in the right range. It doesn't exactly match what I get when I use

\lambda = \frac{2 \mu}{\rho \bar{c}}

where $\mu$ is the viscosity and $\bar{c}$ is the average thermal speed, but that is pretty normal when talking about mean free path, it is off by $\sqrt{2}$ so I'm not too worried. The viscosity form above is the more common formula vs. what you're doing in Mutation++, and I can't seem to find your form in any reference, but we'll keep it for now. I still feel wonder though if it should be Q22 instead of Q11, but we can save that discussion for another day. Closes #239.

Once I fixed the mean free path, I was able to fix the original issue with the Python interface (#221) and update the test value.

I'll give folks a few days to look it over before I click the merge button, unless someone else wants to do it.

mgoodson-cvd commented 10 months ago

Hearing no objections, I will go ahead and merge this.