MFlowCode / MFC

Exascale simulation of multiphase/physics fluid dynamics
https://mflowcode.github.io
MIT License
132 stars 56 forks source link

Add `examples/` into CI #474

Open sbryngelson opened 2 weeks ago

sbryngelson commented 2 weeks ago

Make each example case sufficiently small such that it can run on one processor for at least a few time steps. Then, add a few time steps of each example case to CI and set I/O to write at the end. Then check output for NaNs/Inftys.

We grow the size of the test suite this way, though it isn't as thorough a test as there will be no goldenfiles (though I suppose we could add them).

henryleberre commented 5 hours ago

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: https://github.com/henryleberre/ChemMFC/tree/henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

sbryngelson commented 3 hours ago

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: henryleberre/ChemMFC@henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Thanks @henryleberre! very nice.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

Why so? I think I see some upside to your position. But if you loop through all of them then we are sure that anytime someone adds an example to version control it will get checked, and the developer doesn't have to add the case to a .yaml file somewhere (extra work, very easy to forget, requires some documentation, etc.). Also, I like having many examples (and want more 😄 ), so trying to avoid friction there when reasonable.

sbryngelson commented 3 hours ago

I see that I get NaN on my local machine in the timestep performance output for 1d_impact (haven't checked other cases).

 [ 99%]  Time step      198 of 201 @ t_step = 197
 [ 99%]  Time step      199 of 201 @ t_step = 198
 [100%]  Time step      200 of 201 @ t_step = 199
 Performance:                        NaN  ns/gp/eq/rhs

+-----------------------------------------------------------------------------------------------------------+
| Finished MFC:                                                                                             |
| * Total-time:    0s                                  * Exit Code:     0                                   |
| * End-time:      08:38:16                            * End-date:      08:38:16                            |
+-----------------------------------------------------------------------------------------------------------+

mfc: (venv) Exiting the Python virtual environment.

Also something about the new node20 requirements for github action checkout is killing our phoenix runner.

Note that this case doesn't appear to have failed, just the performance counter did (I think).

Update: I'm fixing node20 requirements on phoenix via pr #505

henryleberre commented 2 hours ago

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: henryleberre/ChemMFC@henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Thanks @henryleberre! very nice.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

Why so? I think I see some upside to your position. But if you loop through all of them then we are sure that anytime someone adds an example to version control it will get checked, and the developer doesn't have to add the case to a .yaml file somewhere (extra work, very easy to forget, requires some documentation, etc.). Also, I like having many examples (and want more 😄 ), so trying to avoid friction there when reasonable.

I am not entirely opposed to this. My reasoning is that some cases require special attention and may not work out of the box with this kind of hacking.

For example, in the above-referenced commit, I hardcoded an exception for the "scaling" test case since it requires the user to input parameters. We could currently bypass this by changing the case to assume sensible defaults. For this case in particular, the best way to make it a test would be to pass arguments to to use a very small number of points (since it is a configurable parameter). You could argue that this configuration should be the default..

We have to see what @okBrian finds to make an educated decision.

henryleberre commented 2 hours ago

I see that I get NaN on my local machine in the timestep performance output for 1d_impact (haven't checked other cases).

 [ 99%]  Time step      198 of 201 @ t_step = 197
 [ 99%]  Time step      199 of 201 @ t_step = 198
 [100%]  Time step      200 of 201 @ t_step = 199
 Performance:                        NaN  ns/gp/eq/rhs

+-----------------------------------------------------------------------------------------------------------+
| Finished MFC:                                                                                             |
| * Total-time:    0s                                  * Exit Code:     0                                   |
| * End-time:      08:38:16                            * End-date:      08:38:16                            |
+-----------------------------------------------------------------------------------------------------------+

mfc: (venv) Exiting the Python virtual environment.

Also something about the new node20 requirements for github action checkout is killing our phoenix runner.

Note that this case doesn't appear to have failed, just the performance counter did (I think).

I see the same behavior in my chemistry runs. I haven't looked into it yet.

sbryngelson commented 2 hours ago

@okBrian after talking with @sbryngelson about this, I made a branch with a commit to help kickstart this: henryleberre/ChemMFC@henry/474. The code adds a new test case for each example. I added a feature where you can specify a functor that lets you modify the case dictionary generated after reading the input file.

Thanks @henryleberre! very nice.

Note: I would advocate for specifying the list of example cases manually, rather than looping through the folders in the examples folder.

Why so? I think I see some upside to your position. But if you loop through all of them then we are sure that anytime someone adds an example to version control it will get checked, and the developer doesn't have to add the case to a .yaml file somewhere (extra work, very easy to forget, requires some documentation, etc.). Also, I like having many examples (and want more 😄 ), so trying to avoid friction there when reasonable.

I am not entirely opposed to this. My reasoning is that some cases require special attention and may not work out of the box with this kind of hacking.

For example, in the above-referenced commit, I hardcoded an exception for the "scaling" test case since it requires the user to input parameters. We could currently bypass this by changing the case to assume sensible defaults. For this case in particular, the best way to make it a test would be to pass arguments to to use a very small number of points (since it is a configurable parameter). You could argue that this configuration should be the default..

We have to see what @okBrian finds to make an educated decision.

It seems reasonable that any case that takes command-line arguments should also have sensible defaults if no arguments are passed (presumably ones that run quickly on a laptop).