libantioch / antioch

C++ Chemical Kinetics, Thermodynaimics, and Transport Library
https://libantioch.github.io/
Other
22 stars 17 forks source link

Kin/orders reactions #172

Closed SylvainPlessis closed 8 years ago

SylvainPlessis commented 8 years ago

Here's the PR for partial kinetics order, forward and backward, xml and ChemKin format, as needed in issue #163.

The sum is required (like, really required) to be integer, because chemistry, and for the proper calculations of the pre-exponential parameter's unit.

Changes are quite simple, Reaction and ReactionSet are int-or-double-who-really-cares partial orders compatible, the parsers (xml, chemkin and the high-level _read_reaction_setdata function) updated. To ease a little bit the code, the methods in _stringutils.h for parsing a string on a colon (the late _parse_string_int_oncolon and _parse_string_double_ontcolon) have been updated to use a functor, so there's less going around in the code (only the xml parser) to obtain the proper type.

On this subject, I've put the functor in the _stringutils.h file, but I'm not really sure if _metaprogrammingdecl.h wouldn't be a better choice.

The ChemKin keywords are the ones already mentioned in the corresponding issue, I've kept the very same for xml, except that they are in lower case (as all the other xml keywords). Examples are in _test/input_files/orderschemkin.inp and _test/input_files/ordersxml.inp.

This PR is rebased on PR #171.

@beanSnippet, I'd like to have your feedback, does it solve completely the issue?

Happy new year, btw,

fun, alcohol and chocolate :cocktail:

pbauman commented 8 years ago

Could you please rebase from master since I merged #171? Thanks.

SylvainPlessis commented 8 years ago

rebased

rebeccaem commented 8 years ago

Hi guys,

Sorry for lack of response. (I defend in three days, so it's been a little hectic!) I haven't forgotten about this though, and will try it out soon. Thanks for adding it, it's a pretty nice feature to have.

SylvainPlessis commented 8 years ago

Wooohoooo :bowtie: !!

Well indeed it's ununderstandable how you don't have free time to do something else than preparing your defense :smile:.

Good luck (but I personnaly bet on skills), and well, this PR won't move, nothing really urgent here (at least from my side).

pbauman commented 8 years ago

(I defend in three days, so it's been a little hectic!)

Great news, @beanSnippet! Certainly not a problem. What I think would be the most helpful thing for us is if you could supply us a mechanism+program that illustrates improper behavior before this PR and (hopefully) proper behavior after the PR. Hopefully that's not too much work and it certainly can wait until your defense is done and dissertation submitted.

@SylvainPlessis, this made me think: is there a place where the ChemKin format is documented? We should try and detect any "features" that are there and error out if we don't support them, instead of silently giving the wrong answers, which I believe is what happened to @beanSnippet early on.

Otherwise, the change set itself looked OK on first glance, but would be nice to absorb a test case from @beanSnippet.

SylvainPlessis commented 8 years ago

@SylvainPlessis, this made me think: is there a place where the ChemKin format is documented? We should try and detect any "features" that are there and error out if we don't support them, instead of silently giving the wrong answers, which I believe is what happened to @beanSnippet early on.

No I'm afraid, nothing more that the comments on the _chemkinparser.C and _chemkinparser.h.

I never took the time to develop the ChemKin format in the model docs, no hope from there. I do have a pdf of the ChemKin format that I didn't put on the external_doc folder (license and stuff so I don't know what I'm supposed to be able to do or not with the pdf).

Now the error that hit her was way worse than just a non-supported feature: it was a misunderstanding from my part of how the format was (naively, I though it was logical and as simple as possible...). So the code was alright, the Antioch version of the ChemKin format was not.

pbauman commented 8 years ago

I'd like to go ahead and merge this. @hovr2pi and @dsondak are going to play with GRI 3. We can fix more issues as they pop up.

One question though, for @SylvainPlessis: Did you use keywords for the forward and backward order that are cross-compatible with Cantera?

pbauman commented 8 years ago

Actually, @SylvainPlessis, are we cross-compatible with Cantera on the falloff reaction input? It looks different than ours. (See the GRI 3.0 xml file cantera ships in their data/inputs directory.)

SylvainPlessis commented 8 years ago

It doesn't look like we are. The falloffs in this file are coded less efficiently than Antioch (so I guess lazyness was the killer here). They just give 'falloff' as a type and thus require whatever happens a tag for precising what kind of falloff, and when needed some data (and thus more tags and keys... Efficiency, where art thou?).

They also have a hard-coded way of providing the Troe's falloff parameters, we need to know what parameter is what.

Those are the only differences I'm seeing, should not be complicated to achieve cross-compatibility.

For a better view, here is the Cantera way

    <reaction reversible="yes" type="falloff" id="0050">
      ... stuff ...
      <rateCoeff>
        ... stuff ...
        <efficiencies default="1.0">AR:0.7  C2H6:3  CH4:2  CO:1.5  CO2:2  H2:2  H2O:6 </efficiencies>
        <falloff type="Troe">0.562 91 5836 8552 </falloff>
      </rateCoeff>
       ... stuff ...
    </reaction>

This would be the Antioch way:

    <reaction reversible="yes" type="TroeFalloff" id="0050">
      ... stuff ...
      <rateCoeff>
        ... stuff ...
        <efficiencies default="1.0">AR:0.7  C2H6:3  CH4:2  CO:1.5  CO2:2  H2:2  H2O:6 </efficiencies>
        <Troe>
          <alpha>0.562</alpha> 
           <T1 || T2 || T3>91 </T1 || T2 || T3>
           <T1 || T2 || T3>5836  </T1 || T2 || T3>
           <T1 || T2 || T3>8552 </T1 || T2 || T3>
       </Troe>
      </rateCoeff>
       ... stuff ...
    </reaction>

Edit: manipulation error

pbauman commented 8 years ago

Thanks @SylvainPlessis. I agree, shouldn't be hard to achieve cross-compatibility. We can do that in a separate PR.

Any idea about the reaction order?

SylvainPlessis commented 8 years ago

I've forgotten that part. Actually I haven't seen this in a Cantera file, so I just used the ChemKin keyword in lower case. If you find them, I think we'd better use the same and change them if needed (should take between 1 and 5 minutes, plus compilation/check time).

pbauman commented 8 years ago

Yeah, I was just looking. They don't have any in example files, but in their code it looks like there's an order keyword in there, but I just glanced. In that case, I think we move forward here and once @beanSnippet has some time, we can get her input and try and to reverse engineer what Cantera is doing and confirm we get similar results. I'll open issues for the order and the falloff bits. Thanks for your quick reply @SylvainPlessis.

pbauman commented 8 years ago

Replaced by #181.