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.

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

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.

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

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

@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 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?

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

[...] 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).

The "species lock" issue is resolved by 119af47 in #1864.

Ignoring NotImplementedErrors from intEnergy_mass was resolved by 4446f76 in #1878.

However, solving reactors using PlasmaPhase with the energy equation enabled requires implementations for cp_mole and/or cv_mole, either directly for the evaluation of the governing equations (in the case of the "ideal gas" reactor variants) or as part of the algorithm for setting the enthalpy / internal energy (in the general Reactor or ConstPressureReactor variants), which means the correct resolution of this issue is via #1872.