search

dougshidong / PHiLiP

Parallel High-Order Library for PDEs through hp-adaptive Discontinuous Galerkin methods
Other
45 stars 37 forks source link

FlowSolver standalone class #147

Closed jbrillon closed 2 years ago

jbrillon commented 2 years ago

Pull request to close issue #120. Introducing the run_type parameter for running either: integration_test or flow_simulation.

jbrillon commented 2 years ago

One point of discussion: could we move the tests which use flow_solver into a subdirectory? E.g., testing/flow_solver_tests. This would make it easier to distinguish the tests that use flow_solver from those that don't. What do you think?

Ideally, we would want all the older tests to take advantage of the flow_solver for taking care of getting the flow solution, greatly reducing code repetition, so that would mean they all fall under the src/testing/ directory. I think its fine to leave it as is since we know that this is an existing issue we want to address in the future. Or we could introduce two subdirectories for reorganizing the (1) src/testing/new_tests/, and (2) src/testing/old_tests/; where we'd eventually want to move the old tests into the new tests once they've been changed to use flow solver. @dougshidong @donovan97 what do you think?

dougshidong commented 2 years ago

Nice clean up.

The testing file and folder restructure is a known improvement to be done. Feel free to open an issue about it @cpethrick .

The current will go well with the issue I just opened for a future clean up using ctest labels: https://github.com/dougshidong/PHiLiP/issues/148

One thing to keep in mind is that a flow simulation can also be an integration test. We need to be careful about pulling out tests out of the integration test suite even if it uses flow solver.

@cpethrick since you seem to have reviewed the code as well, please feel free to approve and merge it if you're satisfied with his answer. I'm trying to stop approving PR as much as possible and leaving it up to you all to approve each others PR.