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

Enhance the PlasmaPhase to use the cross section data with an example of calculating the elastic collision energy loss rate #1731

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

BangShiuh
Copy link
Contributor

@BangShiuh BangShiuh commented Jul 8, 2024

Changes proposed in this pull request
This PR change the initiation of the PlasmaPhase. The PlasmaPhase after this change takes the ElectronCollisionPlasmaRate cross-section data to calculate the plasma thermal properties (specifically the elastic collision energy loss rate). The elastic collision energy loss is the energy transfer from electron to molecule when a fast electron (higher electron temperature) collides to a slower molecule (lower gas temperature). This is useful for modelling a non-equilibrium plasma in which the electron temperature is much higher than the gas temperature. The elastic collision power loss includes the energy transfer via elastic collision and via the recoil effect of an inelastic collision. The equation can be found in the manual of bolsig, https://www.bolsig.laplace.univ-tlse.fr/download.html. I have compared the value of elastic power loss to bolsig, and it is reasonably close (around 1% different).

Data download:
The collision cross section can be downloaded from https://nl.lxcat.net/data/download.php. However, the zip file is currently broken. Fortunately, I have saved a previous version. Please email me if you want to use it.

If applicable, fill in the issue number this pull request is fixing

Closes #

If applicable, provide an example illustrating new features this pull request is introducing

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

@BangShiuh BangShiuh changed the title Collision Enhance the PlasmaPhase to use the cross section data with an example of calculating the elastic collision energy loss rate Jul 8, 2024
@BangShiuh BangShiuh changed the title Enhance the PlasmaPhase to use the cross section data with an example of calculating the elastic collision energy loss rate Enhance the PlasmaPhase to use the cross section data with an example of calculating the elastic collision energy loss rate (WIP) Jul 8, 2024
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 77.19298% with 39 lines in your changes missing coverage. Please review.

Project coverage is 73.26%. Comparing base (80c0bb2) to head (9a46ebb).

Files with missing lines Patch % Lines
src/thermo/PlasmaPhase.cpp 83.73% 8 Missing and 12 partials ⚠️
include/cantera/kinetics/ReactionRate.h 0.00% 12 Missing ⚠️
src/kinetics/Kinetics.cpp 40.00% 2 Missing and 1 partial ⚠️
include/cantera/kinetics/Kinetics.h 50.00% 2 Missing ⚠️
src/kinetics/Reaction.cpp 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1731      +/-   ##
==========================================
+ Coverage   73.23%   73.26%   +0.02%     
==========================================
  Files         383      383              
  Lines       54652    54799     +147     
  Branches     9109     9133      +24     
==========================================
+ Hits        40027    40150     +123     
- Misses      11612    11625      +13     
- Partials     3013     3024      +11     

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

@BangShiuh BangShiuh changed the title Enhance the PlasmaPhase to use the cross section data with an example of calculating the elastic collision energy loss rate (WIP) Enhance the PlasmaPhase to use the cross section data with an example of calculating the elastic collision energy loss rate Jul 8, 2024
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @BangShiuh. I had some suggestions that I think will lead to a simpler implementation. Please let me know if you have any questions.

interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pxd Outdated Show resolved Hide resolved
interfaces/cython/cantera/thermo.pyx Outdated Show resolved Hide resolved
include/cantera/thermo/PlasmaPhase.h Outdated Show resolved Hide resolved
test/python/test_thermo.py Outdated Show resolved Hide resolved
src/thermo/PlasmaPhase.cpp Outdated Show resolved Hide resolved
include/cantera/thermo/ThermoPhase.h Outdated Show resolved Hide resolved
include/cantera/thermo/ThermoPhase.h Outdated Show resolved Hide resolved
src/base/Solution.cpp Outdated Show resolved Hide resolved
src/thermo/PlasmaPhase.cpp Outdated Show resolved Hide resolved
@speth
Copy link
Member

speth commented Nov 13, 2024

@BangShiuh, I saw that you have marked all the comments from my previous review as "resolved", but I think the corresponding changes have not been pushed to this branch. Are they all in your collision-new branch that you mentioned in Cantera/enhancements#194? Could you push them to this branch?

@BangShiuh
Copy link
Contributor Author

BangShiuh commented Nov 13, 2024 via email

@BangShiuh BangShiuh force-pushed the collision branch 2 times, most recently from bde326e to 5f00629 Compare November 19, 2024 04:33
@BangShiuh
Copy link
Contributor Author

@speth I have made updating rate or adding reaction triggers updating the interpolated cross sections. I also have tried to reduce the number of interpolating the cross-sections, but not successful. The rate object in PlasmaPhase is different to the one in m_bulk_rates, which is used by updateROP.

I will check the Linters checks failure. How do I run that on my computer?

@speth
Copy link
Member

speth commented Nov 19, 2024

I will check the Linters checks failure. How do I run that on my computer?

The easiest thing to do is just look at the logs here on GitHub. The problem that it is finding is with the docstring for class PlasmaPhase, which starts with:

/*!
 * Base class for handling plasma properties, specifically focusing on the
 * electron energy distribution. This class provides functionality
 * to manage the the electron energy distribution using two primary methods
 * for defining the electron distribution and electron temperature.
 *

There is a subtle difference between starting a Doxygen comment block with /** and /*!. In the former case, the first sentence of the comment block (that is, up to the first period or blank line) will be used as the brief description. In the latter case, it will not. Since we often rely on this feature for providing the brief description, the comment block should start with /** instead. The other way to write this with the brief description more explicitly provided is:

//! Base class for handling plasma properties, specifically focusing on the
//! electron energy distribution. 
/*!
 * This class provides functionality to manage the the electron energy distribution
 * using two primary methods for defining the electron distribution and electron
 * temperature.
 *

@speth
Copy link
Member

speth commented Dec 12, 2024

Commit 21bbebd erroneously reverts the state of the example_data submodule. If you undo that change, that should fix the failing CI jobs.

@BangShiuh
Copy link
Contributor Author

I am not sure why 4e7d6e2 is [1D].... I will investigate it.

@speth
Copy link
Member

speth commented Dec 12, 2024

I think you've accidentally squashed a commit of yours into one of mine. The only change that should be in the commit with that message is a 3-line change to Flow1D.cpp. You might be able to resolve it by rebasing onto the current main branch, and then just editing the commit message for that commit.

@BangShiuh
Copy link
Contributor Author

I used reset during rebase to remove the change to example_data. But I used commit --amend, so the original commit was gone and became [1D] ... should just use commit instead.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks for the updates here, @BangShiuh. I think this is just about ready -- I just have a few comments that are mostly about the documentation and code style.

Comment on lines +34 to +35
* distribution (as described by @cite gudmundsson2001 @cite khalilpour2020)
* is given by:
Copy link
Member

Choose a reason for hiding this comment

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

Because the @cite command only produces the numerical reference (for example [10]), write this as:

Suggested change
* distribution (as described by @cite gudmundsson2001 @cite khalilpour2020)
* is given by:
* distribution (as described by Gudmundsson @cite gudmundsson2001 and Khalilpour and Foroutan @cite khalilpour2020)
* is given by:

Comment on lines 37 to +38
* f(\epsilon) = c_1 \frac{\sqrt{\epsilon}}{\epsilon_m^{3/2}}
* \exp(-c_2 (\frac{\epsilon}{\epsilon_m})^x),
* \exp(-c_2 \left(\frac{\epsilon}{\epsilon_m}\right)^x),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* f(\epsilon) = c_1 \frac{\sqrt{\epsilon}}{\epsilon_m^{3/2}}
* \exp(-c_2 (\frac{\epsilon}{\epsilon_m})^x),
* \exp(-c_2 \left(\frac{\epsilon}{\epsilon_m}\right)^x),
* f(\epsilon) = c_1 \frac{\sqrt{\epsilon}}{\epsilon_m^{3/2}}
* \exp\left(-c_2 \left(\frac{\epsilon}{\epsilon_m}\right)^x\right),

Likewise below in the definition of $F(\epsilon)$.

Comment on lines +116 to +118
//! @param updateEnergyDist update electron energy distribution
void setElectronEnergyLevels(const double* levels, size_t length,
bool updateEnergyDist);
Copy link
Member

Choose a reason for hiding this comment

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

Can this be simplified by just adding updateEnergyDist=true as an optional argument to the existing version of setElectronEnergyLevels?

virtual void addSolution(std::weak_ptr<Solution> soln) override;

/**
* The elastic power loss(J/s/m3)
Copy link
Member

Choose a reason for hiding this comment

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

Missing a space. I've also started to prefer writing subscripts and superscripts for units using the corresponding unicode characters, though you can also stick to just m^3 for now if you prefer.

Suggested change
* The elastic power loss(J/s/m3)
* The elastic power loss (J/s/m³)

Comment on lines +419 to +423
//! update the elastic electron energy loss coefficient of i collision
void updateElasticElectronEnergyLossCoefficient(size_t i);

//! update elastic electron energy loss coefficients
void updateElasticElectronEnergyLossCoefficients();
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on the conditions where these functions might need to be called?

Comment on lines 705 to +708

for (const auto& [id, callback] : m_reactionChangedCallbacks) {
callback();
}
Copy link
Member

Choose a reason for hiding this comment

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

Based on where this is called, and how it is used in conjunction with PlasmaPhase, it might be more accurate to name this a reactionAddedCallback.

Comment on lines +333 to +334
// register callback when reaction is added later
// The modifyReaction
Copy link
Member

Choose a reason for hiding this comment

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

Comment here seems incomplete.

@@ -8,9 +8,15 @@
#include "cantera/thermo/Species.h"
#include "cantera/base/global.h"
#include "cantera/numerics/funcs.h"
#include "cantera/kinetics/Kinetics.h"
#include "cantera/kinetics/Reaction.h"
#include "cantera/kinetics/KineticsFactory.h"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need KineticsFactory.h here, do you?


namespace Cantera {

static const double gamma = sqrt(2 * ElectronCharge / ElectronMass);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than static const double, this would be better as just const double in an unnamed namespace.


if (shared_ptr<Solution> soln = m_soln.lock()) {
shared_ptr<Kinetics> kin = soln->kinetics();
if (!kin) return;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use the one-line version of the if statement -- always use braces and put the body on a separate line.

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