search

SimVascular / svFSIplus

This repository contains a line-by-line conversion of the Fortran svFSI solver to C++.
Other
6 stars 19 forks source link

Precomputed velocities #202 #208

Closed zasexton closed 2 months ago

zasexton commented 2 months ago

Current situation

Addressing issue #202

Testing

New testing code has been mentioned in #202

Code of Conduct & Contributing Guidelines

zasexton commented 2 months ago

For some reason there are test integration errors for ubuntu 22.04. These errors are not a result of my code and seem to be from XML byte errors when reading new Unstructured meshes that have been added to svFSIplus.

zasexton commented 2 months ago

Will need some help determining how to resolve the ubuntu-22.04 integration errors.

MatteoSalvador commented 2 months ago

@zasexton I have changed the workflow for tests on Ubuntu to a Docker-based framework. If you pull the current SimVascular/svFSIplus we should be able to review/merge this PR.

zasexton commented 2 months ago

@MatteoSalvador I've pulled the current main branch of svFSIplus.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 90.99099% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 63.12%. Comparing base (6dd920b) to head (8f17cd1).

Files Patch % Lines
Code/Source/svFSI/main.cpp 86.79% 7 Missing :warning:
Code/Source/svFSI/vtk_xml_parser.cpp 87.17% 5 Missing :warning:
Code/Source/svFSI/all_fun.cpp 89.47% 4 Missing :warning:
Code/Source/svFSI/initialize.cpp 88.23% 2 Missing :warning:
Code/Source/svFSI/vtk_xml.cpp 92.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #208 +/- ## ========================================== + Coverage 62.91% 63.12% +0.21% ========================================== Files 105 105 Lines 27214 27424 +210 ========================================== + Hits 17122 17312 +190 - Misses 10092 10112 +20 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ktbolt commented 2 months ago

All of this new code from lines 248-292 should be put into a function.

zasexton commented 2 months ago

Fantastic job @zasexton! I really like the way you handled issue #202, code structure/documentation and comments for newly added features. This is another good example after @aabrown100-git PR on 3D-0D coupling of how things should be done within svFSIplus. To really incorporate all the best practices, I kindly ask you to add a simple test for the new features, so that we also get proper code coverage, and address the minor comments I left. Then, I will be more than happy to merge the PR.

Adding a test case for code coverage in zasexton:precomputed-velocities-#202 as cases/fluid/precomputed_dye_AD

zasexton commented 2 months ago

All of this new code from lines 248-292 should be put into a function.

I have encapsulated the precomputed time advancement interpolation scheme within the new function iterate_precomputed_time and removed that code block from the iterate_solution function.

zasexton commented 2 months ago

@MatteoSalvador I think this pull request should be fine to merge unless there are other comments that need to be resolved

MatteoSalvador commented 2 months ago

Thank @zasexton! It looks good to me now