Skip to content

Conversation

@mquiram
Copy link
Contributor

@mquiram mquiram commented Dec 10, 2025

Changes proposed in this pull request

  • Added Joule heating and elastic power loss terms to energy equation RHS in all reactor classes when thermo: plasma
  • Added PlasmaPhase methods for cp_mole(), intEnergy_mole(), entropy_mole(), and gibbs_mole() which account for a separate electron temperature. Currently, they are commented out and inherited from IdealGasPhase so that the energy equation can work and all the tests can pass. Not sure if we want to maintain inheritance from IdealGasPhase, skip the test, or maybe change the h = u + pv test for PlasmaPhase to something like h = u + pv + X_e(R(T_e - T)) and similarly change the Gibbs consistency test to account for non-equilibrium electron temperatures. Edit: these methods are now uncommented

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

❌ Patch coverage is 30.76923% with 72 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.66%. Comparing base (94a9fc6) to head (74dc550).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/thermo/PlasmaPhase.cpp 48.83% 21 Missing and 1 partial ⚠️
src/equil/MultiPhase.cpp 50.00% 6 Missing ⚠️
src/zeroD/ConstPressureMoleReactor.cpp 0.00% 5 Missing ⚠️
src/zeroD/ConstPressureReactor.cpp 0.00% 5 Missing ⚠️
src/zeroD/IdealGasConstPressureMoleReactor.cpp 0.00% 5 Missing ⚠️
src/zeroD/IdealGasConstPressureReactor.cpp 16.66% 5 Missing ⚠️
src/zeroD/IdealGasMoleReactor.cpp 0.00% 5 Missing ⚠️
src/zeroD/IdealGasReactor.cpp 0.00% 5 Missing ⚠️
src/zeroD/MoleReactor.cpp 0.00% 5 Missing ⚠️
src/zeroD/Reactor.cpp 0.00% 5 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2068      +/-   ##
==========================================
- Coverage   76.75%   76.66%   -0.09%     
==========================================
  Files         457      457              
  Lines       53744    53825      +81     
  Branches     9122     9135      +13     
==========================================
+ Hits        41250    41266      +16     
- Misses       9386     9454      +68     
+ Partials     3108     3105       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@BangShiuh BangShiuh left a comment

Choose a reason for hiding this comment

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

@mquiram Thanks for working on this. I left some comments for the changes in class PlasmaPhase.

}
return m_eedfSolver->getElectronMobility();
} else {
throw NotImplementedError("PlasmaPhase::electronMobility",
Copy link
Contributor

Choose a reason for hiding this comment

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

Electron mobility can be supported without Boltzmann-two-term if we provide the cross-section data and the EEDF. The equation is different according to the bolsig document. I am fine with the current implementation. But I would make another PR to improve this so that the plasma reactor can be more flexible. In addition, IonGasTransport.::getMobilities can use the electron mobility. We can cache the electron mobility value in PlasmaPhase so we don't need to call getElectronMobility() when it stays the same value.

Comment on lines 743 to 758
// sigma = e * n_e * mu_e [S/m]; q_J = sigma * E^2 [W/m^3]
const double mu_e = electronMobility(); // m^2 / (V·s)
if (!(mu_e > 0.0) || !std::isfinite(mu_e)) {
return 0.0;
}
const double ne = concentration(m_electronSpeciesIndex) * Avogadro; // m^-3
if (!(ne > 0.0) || !std::isfinite(ne)) {
return 0.0;
}
const double E = electricField(); // V/m
if (!std::isfinite(E) || E == 0.0) {
return 0.0;
}
const double sigma = ElectronCharge * ne * mu_e; // S/m
return sigma * E * E; // W/m^3
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think checking std::isfinite for each term is needed. The error should be caught when calling the function. We can check if any term is or smaller than zero and return zero..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This, along with lines 577-579 is a sort of debugging artifact, and I agree that it's now unnecessary looking back at it.

Comment on lines 577 to 579
if (!std::isfinite(q_elastic)) {
return 0.0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? Can we issue an error instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants