-
Notifications
You must be signed in to change notification settings - Fork 51
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
Surface Chemistry - Modified mechanism.H preprocessor directives & mechanism.cpp reaction maps #473
Surface Chemistry - Modified mechanism.H preprocessor directives & mechanism.cpp reaction maps #473
Conversation
Only one of two chemistry arguments are accepted namely homogeneous and heterogeneous.
mechanism.H now explicitly incorporates surface information
to species_info.py and the set_species method in converter.py
Essentially surface species, e.g. H(S), are now being defined as H_S_ID instead of HS_ID
to use NUM_REACTIONS instead of getting n_reactions
ceptr.py: chemistry argument is not required and defaults to homogeneous reaction_info: improved verbosity & rewrote isinstance based conditions converter.py: used ternary operator, list comprehension & merged site density definition to one line Also conformed to black and flake8 formatting
converter.py: used ternary operator for Converter.species_info.n_all_species reaction_info.py: improved readability of if conditions using and/or keywords
Thank you for doing this!! We typically run in batch mode. Do you think you could make the defaults such that the following works:
|
@marchdf no worries :) Also, a quick question: do the sources for qss mechanisms change every time they are regenerated? |
Thanks for doing that. Before we merge, you should run the batch convert and checkin all the changes that triggers. One of them I noticed needs to be changed:
this is incorrect. It should read:
The |
they shouldn't... it's always the same for me on the development branch. If they are diffing every time something is wrong with your setup or the new code. |
Hi @marchdf I think the qss mechanisms in the development branch haven't been updated. I've checked with I think I got confused because the new code gives files with all the changes whereas I was expecting only the changes which the new code affected. |
I just ran it on the |
I don't know if I'm doing something wrong, but I still get diffs on the original development branch.. I created a fresh clone of
I have then verified that the changes from the new code overlaid on top of these only modify the preprocessor directives.. |
How strange... are these a lot of diffs? here's my poetry.lock file. Do you see anything there compared to yours that would indicate a diff? Are your diffs formatting things or actual code stuff? |
Exactly the same outcome even with your lock file. The mechanisms change quite a bit which is why I pointed it out. Does the procedure for qss stuff involve stochastic optimization algorithms? I ask because the differences could be due to a random seed issue. Or it may be a platform-dependent issue since I use an Ubuntu box. |
maybe this is a clang-format problem?
|
That's what I was thinking. If it is not formatting properly with clang-format, you should get messages like Once the QSS mechanisms are committed here, I'll verify that the PeleC and PeleLMeX tests all still pass with the modified mechanisms (I don't see any reason they wouldn't, but it would be good to verify), then we should get this merged. |
I created #475 to ensure the same clang-format is used by everyone running ceptr. |
Still no joy. I regenerated the qss mechanisms on another fresh clone of The newly generated poetry.lock file poetry.lock.txt has newer versions of the dependencies wrt the lock file you shared. I suppose if you ran a I am not sure, but I have a feeling this behavior may be due to different |
Ok I pushed the qss file update. @baperry2 please check and see if all is good now. |
Verified this PR won't break LMeX or C, so I'm going to approve. Sorry this PR has taken unusually long to get through with all the mechanism regeneration stuff. @jAnirudh one last request though, can you update the docs (https://github.com/AMReX-Combustion/PelePhysics/blob/development/Docs/sphinx/Ceptr.rst) with the modified usage that you put in the PR description? |
No worries! I've modified the documentation as well. |
Docs/sphinx/Ceptr.rst
Outdated
|
||
$ cd ${PELE_PHYSICS_HOME}/Support/ceptr | ||
$ poetry run convert -f ${PELE_PHYSICS_HOME}/Mechanisms/LiDryer/mechanism.yaml | ||
|
||
NOTE: CEPTR interpretations of heterogeneous mechanisms is currently a work in progress. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this an actual rst
type note? there are some examples in here somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a new commit with the rst
type note
Added
CEPTR
the reaction mechanism type along with the names of the gas and interface phasespoetry run convert -f ${PELEPHYSICS_HOME}/Mechanisms/${MECHANISM}/mechanism.yaml --chemistry {mechanism-type} --gas_name {gas-name} --interface_name {interface-name}
-c
instead of--chemistry
mechanism-type
has to be eitherhomogeneous
orheterogeneous
; the default ishomogeneous
gas-name
andinterface-name
aregas
andNone
respectivelyheterogeneous
mechanisms,interface-name
has to be explicitly specifiedpoetry run convert {-l/lq} ${PELEPHYSICS_HOME}/Mechanisms/{list_mech/list_qss_mech}
Converter.set_species
methodreaction_info.py
moduleModified
Converter.mechanism_header_includes
to specify surface elements, species, reactions, site density informationmechanism.cpp
that are generated byReactionInfo.rmap
andReactionInfo.get_rmap
methodsThis work was performed at the FLAME Lab headed by Dr. Konduri Aditya at CDS, IISc Bengaluru with funding from Shell India Markets Pvt. Ltd. and periodic reviews from the Fluid Flow and Reaction Engineering Group at Shell Technology Center Bengaluru.