search

lcpp-org / crane

A MOOSE application dedicated to general Chemical ReAction NEtworks for plasma chemistry and thermochemistry problems.
https://crane-plasma-chemistry.readthedocs.io/
GNU Lesser General Public License v2.1
21 stars 20 forks source link

Restore positivity constraint in ADZapdosEEDFRateConstant and Convert TimeDerivativeLog to AD #73

Closed cticenhour closed 3 years ago

cticenhour commented 3 years ago

The recent removal of the positivity constraint in ADZapdosEEDFRateConstant in #71 caused a failure in the shooting method test in shannon-lab/zapdos#80. This PR restores that constraint.

This PR also converts TimeDerivativeLog to AD (so that ADTimeDerivativeLog in Zapdos can just be removed). Tested as working against current CRANE tests on MacOS with Big Sur 11.2.1

keniley1 commented 3 years ago

Sorry about the removal -- it must have slipped my mind.

But realistically the ADZapdosEEDFRateConstant class should be deprecated in favor of the InterpolatedCoefficientLinear/InterpolatedCoefficientSpline classes. We can leave it in for now since some tests apparently use it, but it's just a duplicate of the InterpolatedCoefficientSpline material.

EDIT: It's also interesting that the positivity constraint actually made a difference...I removed them because I have never run into a situation where the interpolated coefficient was negative. I guess it is important to include!

cticenhour commented 3 years ago

But realistically the ADZapdosEEDFRateConstant class should be deprecated in favor of the InterpolatedCoefficientLinear/InterpolatedCoefficientSpline classes. We can leave it in for now since some tests apparently use it, but it's just a duplicate of the InterpolatedCoefficientSpline material.

Looking at #71 I would agree with this. I'm leaving that to @csdechant though, as he seems to be working with those new classes in his Zapdos improvements branch. I'm just trying to get the AD conversion of the "current Zapdos" done so that I can help integrate his changes with my electromagnetic solver infrastructure stuff that should be coming down the pipe this week or next.

EDIT: It's also interesting that the positivity constraint actually made a difference...I removed them because I have never run into a situation where the interpolated coefficient was negative. I guess it is important to include!

I didn't expect it either, and it honestly took me forever to find it in the shooting method test. I just had some floating point exception error (seemingly a divide by zero) in libmesh after fixing up an issue (of my own making) with a converted kernel class. The error backtrace only took me to the interpolation itself, not any class that was using it, so really not helpful at all. I'm still trying to work out the exact "why", since I don't remember this being used in an exponential (where we might have the negative sign flip the exponential, coupled with a very small value in the exponent leading to a divide by zero).

cticenhour commented 3 years ago

That was my initial working theory after finding that the constraint worked, anyway.