Fix for Issue #385 - Githubissues

MFlowCode / MFC

Exascale simulation of multiphase/physics fluid dynamics

https://mflowcode.github.io

MIT License

132 stars 56 forks source link

Fix for Issue #385 #411

Closed BucketofJava closed 1 month ago

BucketofJava commented 1 month ago

Description

A fix for issue #385, updating time to be recorded in ns/gp/rhs. Also adding benchmark case.py to ./examples.

Related to https://github.com/MFlowCode/MFC/issues/299

Type of change

Please delete options that are not relevant.

[x] Something else

Scope

[x] This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[x] Ran benchmark cases

Test Configuration:

What computers and compilers did you use to test this: Tested locally on WSL Lenovo L380 Yoga Intel(R) Core(TM) i5-8250U CPU @ 1.60GHz

Checklist

[x] This PR does not introduce any repeated code (it follows the DRY principle)
[x] I cannot think of a way to condense this code and reduce any introduced additional line count

sbryngelson commented 1 month ago

Needs to run ./mfc.sh format @BucketofJava

BucketofJava commented 1 month ago

Updated the code according to the comments; For the benchmark case, I moved it to the benchmarks folder, named it "default" as a placeholder and added it to the yml.

sbryngelson commented 1 month ago

You are failing the Pretty CI because you didn't follow my comment above (or in the PR template). How long does it take to run this benchmark case? On CPU? GPU?

sbryngelson commented 1 month ago

This PR doesn't actually compute the other things I mention in the issue:

Update: Probably want to switch both the code and the .md file in the docs to be ns per grid-point per equation per rhs evaluation (I updated the docs to have this already but without the "per-equation" part).

Note that the number of equations is the same as sys_size

BucketofJava commented 1 month ago

Sorry, I did not see the per-equation part before. I have run ./mfc.sh format with every commit, however I did not run it with -j ${nproc} as in the test case, this commit has done this, so hopefully the issue should be fixed.

sbryngelson commented 1 month ago

@BucketofJava if you change the docs markdown page to have per equation then you also need to change all of the performance numbers in that table (dividing them by the number of equations). I think this means dividing them all by 8?

sbryngelson commented 1 month ago

PR merges are on hold until benchmark CI is working (issue #419)

sbryngelson commented 1 month ago

@BucketofJava or someone... can you force sync this fork with MFC root master so I can merge?

sbryngelson commented 1 month ago

@AiredaleDev can you assist with finishing off this PR?

AiredaleDev commented 1 month ago

Sure thing -- it merged cleanly with master with no issue. I'd like to push the changes to this PR but I need access to @BucketofJava 's fork, so I made another PR instead: #432

sbryngelson commented 1 month ago

Superseded by #411