Remove constrains of using PlasmaPhase for 0D reactor and fix the species lock

[BangShiuh]

Changes proposed in this pull request

This change fix the Reactor and ReactorBase so more phase can use reactor in the future. I also make the error message "incompatible phase type provided" visible. We can have more discussion on this and figure out the better solution if needed.

• When the phase is not compatible with the reactor, setThermo triggers the destruction of the reactorBase object which calls removeSpecieslocks(). Looking at ReactorBase::setSolution, the lock is already removed from at line 56. This makes removeSpecieslocks() throw the error. Therefore, I think it is better to check whether locks exist before we remove it.
• For phase like PlasmaPhase, intEnergy_mass is not implemented yet, but we should allow them into a reactor if the energy equation is disabled. Therefore, In setThermo, the assignment of those properties are moved into a try block. (the m_energy is not initiated yet so I use try instead)
• Similarly, I put a guard using m_energy for syncState.

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

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.41%. Comparing base (fce3183) to head (2b99006).
Report is 30 commits behind head on main.

Files with missing lines	Patch %	Lines
src/zeroD/Reactor.cpp	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1860   +/-   ##
=======================================
  Coverage   74.40%   74.41%           
=======================================
  Files         386      386           
  Lines       53630    53633    +3     
  Branches     9065     9066    +1     
=======================================
+ Hits        39905    39910    +5     
+ Misses      10653    10651    -2     
  Partials     3072     3072           

☔ 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.

[speth]

Thanks for looking into this @BangShiuh.

Rather than changing the removeSpeciesLock to ignore inconsistencies, I think what you want to do is update the ReactorBase constructor so that there is no case where m_solution is set but the lock is not added. That is, you want to make sure that there is nothing that can raise exception between the two, either by moving the assignment of m_solution below the try/catch, or the call to addSpeciesLock to be before setThermo.

[BangShiuh]

It seems that some recent commits change the behavior. I will check later.

[ischoegl]

@BangShiuh ... it turns out that I came across some of the same lines in #1864 (which implements a generalization of ReactorBase). I decided to add the bypass of intEnergy_mass there also (in a 'co-authored' commit). This, however, doesn't address the new unit tests, which won't raise the exceptions you specified.

[BangShiuh]

@ischoegl So the current Reactor class does not check the phase type, right? What if a user uses a phase that has issues with a specific reactor?

[ischoegl]

@ischoegl So the current Reactor class does not check the phase type, right? What if a user uses a phase that has issues with a specific reactor?

@BangShiuh ... this is correct. Reactor and MoleReactor are agnostic of the associated phase and assume that ThermoPhase objects are fully functional. If a reactor has issues with a ThermoPhase object, it will throw an exception (NotImplemenedError in your case).

If you want to add special treatment for something that is not implemented, you'd have to create a new Reactor/MoleReator class specialization. Other than that, things may just work If you keep the energy equation disabled (with the fix here or in #1864). Overall, it looks like implementing intEnergy_mass for plasma may be a good option - I assume that the definition of enthalpy $h=u+Pv$ still holds for plasma?

[BangShiuh]

@ischoegl Thanks for the information. I will consider adding some information to warn user in the code or at least in the documentation. (Not adding work for you but just a reminder for myself) The energy equation is tricky for plasma because we have two temperatures. The total internal energy including both gas and electron but this is not the property we want for calculating the gas temperature. I need to either customize the internal energy or make a new variable for the GAS internal energy.

[ischoegl]

[...] I will consider adding some information to warn user in the code or at least in the documentation. (Not adding work for you but just a reminder for myself) [...]

It would be straightforward to catch the exception and raise it again from syncState, which would give the proper context (although it's already provided in the error stack).