AMReX-Combustion / PelePhysics

A collection of physics databases and implementation code for use with the Pele suite of of codes
https://amrex-combustion.github.io/PelePhysics/
Other
60 stars 52 forks source link

Surface Chemistry - Modified mechanism.H preprocessor directives & mechanism.cpp reaction maps #473

Closed jAnirudh closed 8 months ago

jAnirudh commented 9 months ago

Added

  1. User input informing CEPTR the reaction mechanism type along with the names of the gas and interface phases
    • Works for mechanism files (both in single and batch modes)
    • usage for individual files: poetry run convert -f ${PELEPHYSICS_HOME}/Mechanisms/${MECHANISM}/mechanism.yaml --chemistry {mechanism-type} --gas_name {gas-name} --interface_name {interface-name}
      • can also use -c instead of --chemistry
      • mechanism-type has to be either homogeneous or heterogeneous; the default is homogeneous
      • the default values of gas-name and interface-name are gas and None respectively
      • for heterogeneous mechanisms, interface-name has to be explicitly specified
    • default usage for batch conversion of homogeneous reaction mechanism remains unchanged: poetry run convert {-l/lq} ${PELEPHYSICS_HOME}/Mechanisms/{list_mech/list_qss_mech}
  2. Heterogeneous species information in the Converter.set_species method
  3. Indexing for Heterogeneous reactions in the reaction_info.py module

Modified

  1. Converter.mechanism_header_includes to specify surface elements, species, reactions, site density information
  2. reaction map methods in mechanism.cpp that are generated by ReactionInfo.rmap and ReactionInfo.get_rmap methods

This 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.


marchdf commented 9 months ago

Thank you for doing this!!

We typically run in batch mode. Do you think you could make the defaults such that the following works:

poetry run convert -l ${HOME}/combustion/Pele/PelePhysics/Mechanisms/list_mech
Using 10 processes
multiprocessing.pool.RemoteTraceback:
"""
Traceback (most recent call last):
  File "/Users/mhenryde/.pyenv/versions/3.10.11/lib/python3.10/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/Users/mhenryde/.pyenv/versions/3.10.11/lib/python3.10/multiprocessing/pool.py", line 51, in starmapstar
    return list(itertools.starmap(args[0], args[1]))
TypeError: convert() missing 3 required positional arguments: 'chemistry', 'gas_name', and 'interface_name'
"""
jAnirudh commented 9 months ago

@marchdf no worries :)

Also, a quick question: do the sources for qss mechanisms change every time they are regenerated?

marchdf commented 9 months ago

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:

#define NUM_ELEMENTS NUM_GAS_ELEMENTS + NUM_SURFACE_ELEMENTS
#define NUM_SPECIES NUM_GAS_SPECIES + NUM_SURFACE_SPECIES
#define NUM_REACTIONS NUM_GAS_REACTIONS + NUM_SURFACE_REACTIONS

this is incorrect. It should read:

#define NUM_ELEMENTS (NUM_GAS_ELEMENTS + NUM_SURFACE_ELEMENTS)
#define NUM_SPECIES (NUM_GAS_SPECIES + NUM_SURFACE_SPECIES)
#define NUM_REACTIONS (NUM_GAS_REACTIONS + NUM_SURFACE_REACTIONS)

The () are important.

marchdf commented 9 months ago

do the sources for qss mechanisms change every time they are regenerated?

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.

jAnirudh commented 9 months ago

do the sources for qss mechanisms change every time they are regenerated?

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 poetry run -lq ../../Mechanisms/list_qss_mech in the development branch of my fork and I get a few modified mechanism.H files. Would it be possible to verify if this is indeed the case?

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.

marchdf commented 9 months ago

Would it be possible to verify if this is indeed the case?

I just ran it on the development branch and I get no diffs... maybe your development branch is out of date?

jAnirudh commented 9 months ago

I just ran it on the development branch and I get no diffs... maybe your development branch is out of date?

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 PelePhysics, redirected to Support/ceptr and ran poetry update; poetry install. Then I run poetry run convert -lq ../../Mechanisms/list_qss_mechanisms. A git status then gives me

modified:   ../../Mechanisms/C1-C2-NO_qss/mechanism.H
modified:   ../../Mechanisms/CH4_lean_qss/mechanism.H
modified:   ../../Mechanisms/LuEthylene_qss/mechanism.H
modified:   ../../Mechanisms/dodecane_lu_qss/mechanism.H
modified:   ../../Mechanisms/heptane_lu_qss/mechanism.H

I have then verified that the changes from the new code overlaid on top of these only modify the preprocessor directives..

marchdf commented 9 months ago

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? poetry.lock.txt

jAnirudh commented 9 months ago

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.

marchdf commented 9 months ago

maybe this is a clang-format problem?

$ clang-format --version
clang-format version 17.0.6
baperry2 commented 9 months ago

That's what I was thinking. If it is not formatting properly with clang-format, you should get messages like Clang-format not found. C++ files will be hard to parse by a human. in the output, but they may be buried. We should add the clang-format dependency through poetry to avoid this issue in the future.

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.

marchdf commented 9 months ago

I created #475 to ensure the same clang-format is used by everyone running ceptr.

jAnirudh commented 9 months ago

Still no joy. I regenerated the qss mechanisms on another fresh clone of PelePhysics which has the clang-format commit. I've included the files in this zip. The same diffs persist.

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 poetry update and regenerated the mechanisms, they might change to what I'm getting?

I am not sure, but I have a feeling this behavior may be due to different numpy versions giving different values of np.random.seed as described in this SO answer. I see that NetworkX has some documentation on RNGs, so it may well be the case that there is a call to np.random.seedsomewhere.

marchdf commented 9 months ago

Ok I pushed the qss file update. @baperry2 please check and see if all is good now.

baperry2 commented 8 months ago

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?

jAnirudh commented 8 months ago

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.