Aalto-CFD / DLBFoam

DLBFoam: An open-source dynamic load balancing model for fast reacting flow simulations in OpenFOAM. https://doi.org/10.1016/j.cpc.2021.108073, https://doi.org/10.1063/5.0077437
GNU General Public License v3.0
75 stars 27 forks source link

Poor naming of /chemistrySolvers/ode_LAPACK #1

Closed kahilah closed 3 years ago

kahilah commented 3 years ago

At the moment the chemistrySolver type ode_LAPACK is poorly named. The code there is related to pyjac usage only and has nothing to do with Lapack. Hence, renaming should be done for chemistrySolvers/ode_LAPACK

blttkgl commented 3 years ago

I mean in practice the standard chemistry model of OpenFOAM can be used with the custom ODE solver which uses LAPACK. In that sense it makes sense to name it ode_LAPACK, instead of ode_pyJac. However I know that mixing ode_LAPACK with standard chemistry model does not really work (until now). There have been couple of bugfixes to standard Jacobian calculation in OpenFOAM-dev, so I suggest waiting till OpenFOAM-9 is released and check the standard+ode_LAPACK performance.

kahilah commented 3 years ago

I agree that one could wait for OF9. However, that particular file is not related to whether you can use lapack or not, it is there to enable consistent ODE system generation w.r.t species numbering (N-1 in pyjac). Hence, the file naming is not related to lapack but to pyjac.

Using lapack with a standard solver is easily done but prevented by my additional if (yTemp_[0] <= NASAP_mintemp || yTemp_[0] > NASAP_maxtemp) check which assumes pyjac based problem formulation. At least in OF6 lapack did not help the standard solver any bit.

blttkgl commented 3 years ago

Hm, I didn't realize that you were preventing using LAPACK with the standard solvers! That's interesting. In that case I'd propose for OF9 release which is a month away and see if standard+LAPACK provides performance after the recent fixes. For the name convention I see your point now and we can think of a better way. @moreff any thoughts?

moreff commented 3 years ago

Ode solver might be not the best place to check the temperature limits anyway I think. Do we need to check the temperature limits there? Can we move this check to ode_LAPACK or to pyJacLoadBalancedChemistryModel?

Regarding renaming, ode_pyJac sounds fine.

blttkgl commented 3 years ago

It's not about checking temperature limits, it's an explicit block to prevent LAPACK solver to be used with the standard chemistry model. I think we can come back to this after some tests are done with OF-9, we can also fix the naming by then perhaps.

kahilah commented 3 years ago

having the temperature check there is a proper and only way to prevent the ODE solver to go beyonds the thermodynamics limits. That's how it's done in many reference codes so if you have an ODE problem with constraints, the constraints should be taken into account withint the internal loops of the ODE solver as well to achieve correct solution on the first trial without returning anything to the CFD side.

Luckily, when using pyJac, this check is basically never activated due to high accuracy ;) But if you'd use e.g. old OF chemistry, this would flash all the time.

blttkgl commented 3 years ago

Sorry I just misunderstand your statement then. But let's focus on renaming the solver type within this issue for now. As I said if there is a possibility to combo standard+lapack then name would justify itself, however I'm open to ode_pyJac also ofc.