Working 0-1D Chemistry (among other things) by henryleberre · Pull Request #653 · MFlowCode/MFC

henryleberre · 2024-10-16T22:00:34Z

Description

Adds three new working example cases nD_perfect_reactor, 1D_reactive_shocktube, and 1D_inert_shocktube.
Fixes HLL+Advection, Pressure/Energy calculations, and more (for chemistry).
Removes the horrible misc/viz.py script and instead adds a reusable library in the toolchain for this. See the small, individual, viz.py files for each example case I added.
I put some code I often used to remove case parameters for dimensions above 1 or 2 in one file. Cases can just import mfc.case_utils.
For all of this to work, I created a pyproject.toml for the toolchain so that it can be installed in the venv. You can also pip3 install -e toolchain/.
When ./mfc.sh run passes its internal state to the cases it reads, through CLI arguments, it now does so using the --mfc argument (instead of using positional arguments). This improves the usability of the feature because you no longer have to pass your case a fake first argument when running it using python3.
Now that Add C++ & Fortran Backends pyrometheus/pyrometheus#87 and GPU workarounds for OLCF Frontier pyrometheus/pyrometheus#91 have been merged, we point directly to pyrometheus's first release on PyPi.
Refactors the usage of the i[x,y,z] variables everywhere that made the code confusing to read.
While computing the RHS, we no longer perform the unnecessary conversion of the buffer region's conservatives variables to primitives ones. That is already covered by populating the buffer region from the interior points. This greatly improves the performance of the chemistry code since the Newton solve for temperature would fail in this region due to an invalid conservative state.
Augments MFCInputFile refactor & input file-defined tests #410 by correctly (and automatically) handling test cases that need a non-standard build of MFC (for features like Chemistry and Analytically defined patches).
Makes validating case files a lot faster using fastjsonschema. Needed for loading many case files from files (e.g. testing using example cases).
Defers loading/generating cases to after filtering. Improves performance, especially when tests include examples.
Adds test cases for chemistry:

    UUID     Trace
 ─────────────────────────────────────────────────────────────────────────────────────
  5DCF300C   1D -> Chemistry -> Perfect Reactor
  1550B67E   2D -> Chemistry -> Perfect Reactor
  E8372F50   3D -> Chemistry -> Perfect Reactor
  4D7D85B4   1D -> Chemistry -> Inert Shocktube -> Riemann Solver 1 -> Gamma Method 1
  2BDE2018   1D -> Chemistry -> Inert Shocktube -> Riemann Solver 1 -> Gamma Method 2
  6B3AE48F   1D -> Chemistry -> Inert Shocktube -> Riemann Solver 2 -> Gamma Method 1
  F8ADA51B   1D -> Chemistry -> Inert Shocktube -> Riemann Solver 2 -> Gamma Method 2

Fixed the documentation for setting BOOST_INCLUDE on macOS.
HLLC & two ways of calculating $\gamma$.

Type of change

New feature (non-breaking change which adds functionality)

Scope

This PR comprises a set of related changes with a common goal

How Has This Been Tested?

The three example cases recreate simulations from papers referenced in their respective case.py files and READMEs.

sbryngelson

Can you add goldenfiles/tests since we now have examples? Even if they are only for 0D or 1D. this would also help ensure the user has installed boost/pyro and other dependencies correctly.

henryleberre · 2024-10-17T00:00:57Z

Can you add goldenfiles/tests since we now have examples? Even if they are only for 0D or 1D. this would also help ensure the user has installed boost/pyro and other dependencies correctly.

Yes, will do. There is one caveat, however. ./mfc.sh test would build two versions of MFC, the regular, and one using chemistry + h2o2.yml (the reaction mechanism). Are you fine with this? The user need not worry about this happening, they will just see MFC build "twice". Are you fine with this?

codecov · 2024-10-17T00:20:47Z

Codecov Report

Attention: Patch coverage is 66.66667% with 99 lines in your changes missing coverage. Please review.

Project coverage is 42.89%. Comparing base (38a20a7) to head (9b76c7d).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_rhs.fpp	56.86%	22 Missing ⚠️
src/simulation/m_viscous.fpp	63.46%	19 Missing ⚠️
src/simulation/m_time_steppers.fpp	40.74%	16 Missing ⚠️
src/common/m_variables_conversion.fpp	61.53%	12 Missing and 3 partials ⚠️
src/simulation/m_chemistry.fpp	47.36%	9 Missing and 1 partial ⚠️
src/simulation/m_data_output.fpp	14.28%	6 Missing ⚠️
src/simulation/m_riemann_solvers.fpp	91.66%	3 Missing and 1 partial ⚠️
src/post_process/m_start_up.f90	77.77%	2 Missing ⚠️
src/simulation/m_global_parameters.fpp	86.66%	1 Missing and 1 partial ⚠️
src/pre_process/m_assign_variables.fpp	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #653       +/-   ##
===========================================
- Coverage   54.53%   42.89%   -11.65%     
===========================================
  Files          61       61               
  Lines       13802    16070     +2268     
  Branches     1727     1799       +72     
===========================================
- Hits         7527     6893      -634     
- Misses       5819     8191     +2372     
- Partials      456      986      +530

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sbryngelson · 2024-10-17T00:35:00Z

Can you add goldenfiles/tests since we now have examples? Even if they are only for 0D or 1D. this would also help ensure the user has installed boost/pyro and other dependencies correctly.

Yes, will do. There is one caveat, however. ./mfc.sh test would build two versions of MFC, the regular, and one using chemistry + h2o2.yml (the reaction mechanism). Are you fine with this? The user need not worry about this happening, they will just see MFC build "twice". Are you fine with this?

Perhaps... what is the reason? For example, as a user it would be convenient to be able to run a chemistry case, and then switch to a 'normal' case without rebuilding (or vice-versa). Is the mechanism only available at compile time for some reason?

henryleberre · 2024-10-17T01:46:21Z

Can you add goldenfiles/tests since we now have examples? Even if they are only for 0D or 1D. this would also help ensure the user has installed boost/pyro and other dependencies correctly.

Yes, will do. There is one caveat, however. ./mfc.sh test would build two versions of MFC, the regular, and one using chemistry + h2o2.yml (the reaction mechanism). Are you fine with this? The user need not worry about this happening, they will just see MFC build "twice". Are you fine with this?

Perhaps... what is the reason? For example, as a user it would be convenient to be able to run a chemistry case, and then switch to a 'normal' case without rebuilding (or vice-versa). Is the mechanism only available at compile time for some reason?

This is transparent to the user. Chemistry is a compile-time feature because it needs to invoke Pyrometheus to generate code for a specific mechanism file. The build "slug" system is in place to prevent rebuilds for the exact reason you described. The slug is (partially) a function of the mechanism. When you switch back to running a "regular" MFC case, it will re-use a previous build.

    def get_slug(self, case: input.MFCInputFile) -> str:
        if self.isDependency:
            return self.name

        m = hashlib.sha256()
        m.update(self.name.encode())
        m.update(CFG().make_slug().encode())
        m.update(case.get_fpp(self, False).encode())

        if case.params.get('chemistry', 'F') == 'T':
            m.update(case.get_cantera_solution().name.encode())

        return m.hexdigest()[:10]

sbryngelson · 2024-10-17T01:54:01Z

Can you add goldenfiles/tests since we now have examples? Even if they are only for 0D or 1D. this would also help ensure the user has installed boost/pyro and other dependencies correctly.

Yes, will do. There is one caveat, however. ./mfc.sh test would build two versions of MFC, the regular, and one using chemistry + h2o2.yml (the reaction mechanism). Are you fine with this? The user need not worry about this happening, they will just see MFC build "twice". Are you fine with this?

Perhaps... what is the reason? For example, as a user it would be convenient to be able to run a chemistry case, and then switch to a 'normal' case without rebuilding (or vice-versa). Is the mechanism only available at compile time for some reason?

This is transparent to the user. Chemistry is a compile-time feature because it needs to invoke Pyrometheus to generate code for a specific mechanism file. The build "slug" system is in place to prevent rebuilds for the exact reason you described. The slug is (partially) a function of the mechanism. When you switch back to running a "regular" MFC case, it will re-use a previous build.
    def get_slug(self, case: input.MFCInputFile) -> str:
        if self.isDependency:
            return self.name

        m = hashlib.sha256()
        m.update(self.name.encode())
        m.update(CFG().make_slug().encode())
        m.update(case.get_fpp(self, False).encode())

        if case.params.get('chemistry', 'F') == 'T':
            m.update(case.get_cantera_solution().name.encode())

        return m.hexdigest()[:10]

Yes it seems there is no way to avoid this, so long as Pyro is generating our code on the fly for each chemistry case. So yes I am OK with this, as we should probably have a 'store' of mechanism files that we test with located somewhere in VC

henryleberre · 2024-10-17T01:57:08Z

Yes it seems there is no way to avoid this, so long as Pyro is generating our code on the fly for each chemistry case. So yes I am OK with this, as we should probably have a 'store' of mechanism files that we test with located somewhere in VC

I agree. A bunch of standard mechanisms are shipped with Cantera (see https://github.com/Cantera/cantera/tree/main/data). This is why you can run these example cases without downloading a mechanism file.

sbryngelson · 2024-10-21T22:50:08Z

Benchmarking is failing with

      .=++*:          -+*+=.        | sbryngelson3@atl1-1-01-003-34-0.pace.gatech.edu [Linux]
     :+   -*-        ==   =* .      | -------------------------------------------------------
   :*+      ==      ++    .+-       | 
  :*##-.....:*+   .#%+++=--+=:::.   | --jobs 24
  -=-++-======#=--**+++==+*++=::-:. | --mpi --no-gpu --no-debug --no-gcov --no-unified
 .:++=----------====+*= ==..:%..... | --targets pre_process, simulation, and post_process
  .:-=++++===--==+=-+=   +.  :=     | 
  +#=::::::::=%=. -+:    =+   *:    | ----------------------------------------------------------
 .*=-=*=..    :=+*+:      -...--    | $ ./mfc.sh (build, run, test, clean, count, packer) --help


 Benchmarking pre_process, simulation, and post_process (build/benchmarks/e180):
  
   1/4: 5eq_rk3_weno3_hllc @ benchmarks/5eq_rk3_weno3_hllc/case.py
    
     > Log:     build/benchmarks/e180/5eq_rk3_weno3_hllc.out
     > Summary: build/benchmarks/e180/5eq_rk3_weno3_hllc.yaml
     $ ./mfc.sh run /storage/scratch1/6/sbryngelson3/runners/actions-runner-4/_work/MFC/MFC/pr/benchmarks/5eq_rk3_weno3_hllc/case.py --case-optimization --targets pre_process simulation post_process --output-summary /storage/scratch1/6/sbryngelson3/runners/actions-runner-4/_work/MFC/MFC/pr/build/benchmarks/e180/5eq_rk3_weno3_hllc.yaml -c phoenix --gpu -g 0 1 -n 2 -- 1

 

Error: Failed to load YAML from "/storage/scratch1/6/sbryngelson3/runners/actions-runner-4/_work/MFC/MFC/pr/build/benchmarks/e180/5eq_rk3_weno3_hllc.yaml": [Errno 2] No such file or directory: '/storage/scratch1/6/sbryngelson3/runners/actions-runner-4/_work/MFC/MFC/pr/build/benchmarks/e180/5eq_rk3_weno3_hllc.yaml'

Terminated

mfc: ERROR > main.py finished with a 143 exit code.
mfc: (venv) Exiting the Python virtual environment.

the 'master' run doesn't have this error, only the 'pr' run.

henryleberre · 2024-10-21T23:00:49Z

@sbryngelson Thanks for letting me know. Just fixed it.

sbryngelson · 2024-10-23T14:19:37Z

@henryleberre mind pulling from upstream for 'cleanliness'? I made an update there yesterday and want to see how it worked out.

sbryngelson · 2024-10-23T16:56:55Z

@henryleberre pull my 'fix cleanliness' PR so it's up to date with master if possible (that's the thing I would like to test).

henryleberre · 2024-10-24T20:38:41Z

@henryleberre pull my 'fix cleanliness' PR so it's up to date with master if possible (that's the thing I would like to test).

Yes I did, that was the force-push you saw (a rebase on upstream). This is the workflow step that is causing it to fail:
https://github.com/henryleberre/ChemMFC/blob/23df5d6551e7a82217b3259272efe7075d1cf30a/.github/workflows/cleanliness.yml#L99-L103

sbryngelson

🚀🙌🏻

sbryngelson

Testing this is a bit harder because it only appears meaningfully in multi-rank cases. Once you're done, can you test for correctness on a longer 4-rank case, for example, to be sure there isn't a subtle mistake?

sbryngelson · 2024-10-27T00:01:38Z

Testing this is a bit harder because it only appears meaningfully in multi-rank cases. Once you're done, can you test for correctness on a longer 4-rank case, for example, to be sure there isn't a subtle mistake?

Also @henryleberre: It's somewhat unclear what all the different variables really mean (and they don't seem to have comments). I think even after refactor we have 2 or 3 versions of this variable. Can we put comments to say what those are/why they exist?

henryleberre · 2024-10-27T02:28:02Z

Testing this is a bit harder because it only appears meaningfully in multi-rank cases. Once you're done, can you test for correctness on a longer 4-rank case, for example, to be sure there isn't a subtle mistake?

Also @henryleberre: It's somewhat unclear what all the different variables really mean (and they don't seem to have comments). I think even after refactor we have 2 or 3 versions of this variable. Can we put comments to say what those are/why they exist?

I did "document" these like this:

    ! Cell Indices for the interior points (O-m, O-n, 0-p).
    type(int_bounds_info) :: idwint(1:3)
    !$acc declare create(idwint)

    ! Cell Indices for the entire domain, including the buffer region.
    type(int_bounds_info) :: idwbuff(1:3)
    !$acc declare create(idwbuff)

I could maybe have specified these were the proc's local domain. They are only defined in one place now so it's very straightforward to understand. The other indices are module-specific and I'm not sure I have enough context to explain them better than the code does.

henryleberre · 2024-10-27T02:28:47Z

Testing this is a bit harder because it only appears meaningfully in multi-rank cases. Once you're done, can you test for correctness on a longer 4-rank case, for example, to be sure there isn't a subtle mistake?

I'm not sure how this is true. Why are the 2-rank test cases not enough?

sbryngelson · 2024-10-27T02:29:35Z

Testing this is a bit harder because it only appears meaningfully in multi-rank cases. Once you're done, can you test for correctness on a longer 4-rank case, for example, to be sure there isn't a subtle mistake?

I'm not sure how this is true. Why are the 2-rank test cases not enough?

2-rank may be enough, but I'm not sure if our current single test case in the suite is enough.

sbryngelson · 2024-10-27T02:30:59Z

Testing this is a bit harder because it only appears meaningfully in multi-rank cases. Once you're done, can you test for correctness on a longer 4-rank case, for example, to be sure there isn't a subtle mistake?

Also @henryleberre: It's somewhat unclear what all the different variables really mean (and they don't seem to have comments). I think even after refactor we have 2 or 3 versions of this variable. Can we put comments to say what those are/why they exist?

I did "document" these like this:
    ! Cell Indices for the interior points (O-m, O-n, 0-p).
    type(int_bounds_info) :: idwint(1:3)
    !$acc declare create(idwint)

    ! Cell Indices for the entire domain, including the buffer region.
    type(int_bounds_info) :: idwbuff(1:3)
    !$acc declare create(idwbuff)
I could maybe have specified these were the proc's local domain. They are only defined in one place now so it's very straightforward to understand. The other indices are module-specific and I'm not sure I have enough context to explain them better than the code does.

Ah I did not see this part. I mostly saw the module indices. I think those are still confusing. Why do we need any such variables beyond idwint and idwbuff? I can imagine perhaps a couple edge cases but that's about it (like when we do direction reshaping).

old

sbryngelson · 2024-11-04T14:25:53Z

The debug cases are failing on this case (perhaps among others, they only run a fraction of the cases in CI):

 m_mpi_proxy.fpp:1289: @:ALLOCATE_GLOBAL(ib_buff_send(0:-1 + gp_layers * (m + 2*gp_layers + 1)* (n + 2*gp_layers + 1)* (p + 2*gp_layers + 1)/ (min(m, n, p) + 2*gp_layers + 1)))
 m_mpi_proxy.fpp:1301: @:ALLOCATE_GLOBAL(ib_buff_recv(0:ubound(ib_buff_send, 1)))
 m_ibm.fpp:106: @:ALLOCATE_GLOBAL(ghost_points(num_gps))
 m_ibm.fpp:107: @:ALLOCATE_GLOBAL(inner_points(num_inner_gps))
At line 567 of file /Users/runner/work/MFC/MFC/src/simulation/m_ibm.fpp
Fortran runtime error: Index '1' of dimension 1 of array 'ghost_points' above upper bound of -99999918
Error termination. Backtrace:
Could not print backtrace: executable file is not an executable
Could not print backtrace: executable file is not an executable
Could not print backtrace: executable file is not an executable
Could not print backtrace: executable file is not an executable
#0  0x10345ad2f
#1  0x10345b8d7
#2  0x10345bc8f
#3  0x1022f8bdf
#4  0x10230ad67
#5  0x102555d83
#6  0x1026ef79b
#7  0x1026efdfb
mfc: ERROR > :( /Users/runner/work/MFC/MFC/build/install/7e1418ef14/bin/simulation failed with exit code 2.

Case 725A3D56, F13DF273, and 0F6C4340 from the fraction that ran in CI here
https://github.com/MFlowCode/MFC/actions/runs/11660348744/job/32478811683?pr=653#step:7:5812

sbryngelson

this isn't a complete review but covers at least 90% of the diff

Co-authored-by: Dimitrios Adam <mitsos.gr@hotmail.gr>

+ nD_perfect_reactor + 1D_reactive_shocktube + 1D_inert_shocktube

Co-Authored-By: Henry Le Berre <hberre3@gatech.edu>

sbryngelson · 2024-11-04T18:30:57Z

        @:ALLOCATE_GLOBAL(ghost_points(num_gps))
        @:ALLOCATE_GLOBAL(inner_points(num_inner_gps))

+        print *, "num_gps was ", num_gps


henryleberre requested a review from sbryngelson as a code owner October 16, 2024 22:00

henryleberre requested review from sbryngelson and removed request for sbryngelson October 16, 2024 22:00

henryleberre assigned sbryngelson Oct 16, 2024

henryleberre added bug Something isn't working or doesn't seem right enhancement New feature or request labels Oct 16, 2024

sbryngelson reviewed Oct 16, 2024

View reviewed changes

Comment thread src/simulation/m_riemann_solvers.fpp

sbryngelson previously requested changes Oct 16, 2024

View reviewed changes

henryleberre added the documentation Improvements or additions to documentation label Oct 19, 2024

sbryngelson reviewed Oct 26, 2024

View reviewed changes

sbryngelson reviewed Oct 27, 2024

View reviewed changes

sbryngelson self-requested a review October 27, 2024 02:32

henryleberre changed the title ~~Working 0-1D Chemistry~~ Working 0-1D Chemistry (among other things) Oct 27, 2024

henryleberre mentioned this pull request Oct 29, 2024

Fixed Examples, Added Examples CI. #664

Merged

9 tasks

sbryngelson reviewed Nov 4, 2024

View reviewed changes

Comment thread CMakeLists.txt

sbryngelson requested changes Nov 4, 2024

View reviewed changes

henryleberre mentioned this pull request Nov 4, 2024

Add viscous flag instead of any(Re_size > 0) #677

Closed

henryleberre and others added 11 commits November 4, 2024 12:47

Add visualization & case utility libraries

0d7abcb

CheMFC fixes for HLL+Advection & Pressure/Energy

f6e8653

Co-authored-by: Dimitrios Adam <mitsos.gr@hotmail.gr>

Add three 0-1D CheMFC example cases

59e12bb

+ nD_perfect_reactor + 1D_reactive_shocktube + 1D_inert_shocktube

Fix BOOST_INCLUDE path in documentation

b266021

Add CheMFC test cases

2703569

Remove Newton solve for T in buffer regions

0ff25f6

Refactor the use of ix, iy, and iz

85ad653

GPU workarounds for OLCF Frontier

533165e

HLLC Roe speed of sound

574815d

Co-Authored-By: Henry Le Berre <hberre3@gatech.edu>

HLL[C] & Gamma Methods Testing

4ce50f6

Address PR comments

9b76c7d

sbryngelson reviewed Nov 4, 2024

View reviewed changes

This was referenced Nov 4, 2024

Remove chemistry as a fypp macro #678

Closed

Grid index bounds can be collapsed into one overarching derived type #679

Closed

sbryngelson approved these changes Nov 4, 2024

View reviewed changes

sbryngelson merged commit e2ceaf5 into MFlowCode:master Nov 4, 2024

sbryngelson deleted the chemfc-up branch November 4, 2024 20:36

sbryngelson mentioned this pull request Nov 5, 2024

Some cleanup tasks for chemistry following PR #653 #680

Closed

Conversation

henryleberre commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Scope

How Has This Been Tested?

Uh oh!

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

henryleberre commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sbryngelson commented Oct 17, 2024

Uh oh!

henryleberre commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbryngelson commented Oct 17, 2024

Uh oh!

henryleberre commented Oct 17, 2024

Uh oh!

sbryngelson commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryleberre commented Oct 21, 2024

Uh oh!

sbryngelson commented Oct 23, 2024

Uh oh!

sbryngelson commented Oct 23, 2024

Uh oh!

henryleberre commented Oct 24, 2024

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

sbryngelson commented Oct 27, 2024

Uh oh!

henryleberre commented Oct 27, 2024

Uh oh!

henryleberre commented Oct 27, 2024

Uh oh!

sbryngelson commented Oct 27, 2024

Uh oh!

sbryngelson commented Oct 27, 2024

Uh oh!

sbryngelson commented Nov 4, 2024

Uh oh!

Uh oh!

sbryngelson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbryngelson Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

henryleberre commented Oct 16, 2024 •

edited

Loading

henryleberre commented Oct 17, 2024 •

edited

Loading

codecov Bot commented Oct 17, 2024 •

edited

Loading

henryleberre commented Oct 17, 2024 •

edited

Loading

sbryngelson commented Oct 21, 2024 •

edited

Loading