Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kin/orders reactions #172

Closed

Conversation

SylvainPlessis
Copy link
Contributor

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_set_data function) updated. To ease a little bit the code, the methods in string_utils.h for parsing a string on a colon (the late parse_string_int_on_colon and parse_string_double_ont_colon) 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 string_utils.h file, but I'm not really sure if metaprogramming_decl.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/orders_chemkin.inp and test/input_files/orders_xml.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 🍸

@pbauman
Copy link
Member

pbauman commented Jan 2, 2016

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

@SylvainPlessis
Copy link
Contributor Author

rebased

@rebeccaem
Copy link

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
Copy link
Contributor Author

Wooohoooo :bowtie: !!

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

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
Copy link
Member

pbauman commented Jan 18, 2016

(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
Copy link
Contributor Author

@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 chemkin_parser.C and chemkin_parser.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
Copy link
Member

pbauman commented Feb 3, 2016

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
Copy link
Member

pbauman commented Feb 3, 2016

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
Copy link
Contributor Author

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
Copy link
Member

pbauman commented Feb 3, 2016

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
Copy link
Contributor Author

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
Copy link
Member

pbauman commented Feb 3, 2016

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
Copy link
Member

pbauman commented Feb 5, 2016

Replaced by #181.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants