Skip to content

Rework elastic-tube-1d cpp solvers#197

Merged
MakisH merged 38 commits intodevelopfrom
rework-elastictube1d
Apr 14, 2021
Merged

Rework elastic-tube-1d cpp solvers#197
MakisH merged 38 commits intodevelopfrom
rework-elastictube1d

Conversation

@fsimonis
Copy link
Copy Markdown
Member

@fsimonis fsimonis commented Apr 13, 2021

This PR:

  • Modernizes and clarifies the solid-cpp and fluid-cpp solvers
  • Removes memory leaks in both solvers
  • Removes the parallel versions of fluid-cpp and solid-cpp

Closes #194
Closes #193
Closes #195

@fsimonis fsimonis force-pushed the rework-elastictube1d branch from 3901619 to 5fbf26e Compare April 14, 2021 10:43
@fsimonis fsimonis marked this pull request as ready for review April 14, 2021 10:50
@fsimonis fsimonis requested a review from MakisH April 14, 2021 11:02
Copy link
Copy Markdown
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the amazing work! I briefly looked at the code, I did not see anything suspicious, but the diff is of course very large.

While trying to build, I got some warnings for unused variables:

[ 25%] Building CXX object CMakeFiles/FluidSolver.dir/src/FluidSolver.cpp.o
/home/makish/github/tutorials/elastic-tube-1d/fluid-cpp/src/FluidSolver.cpp: In function ‘int main(int, char**)’:
/home/makish/github/tutorials/elastic-tube-1d/fluid-cpp/src/FluidSolver.cpp:32:16: warning: unused variable ‘tau’ [-Wunused-variable]
   32 |   const double tau        = atof(argv[3]);
      |                ^~~
/home/makish/github/tutorials/elastic-tube-1d/fluid-cpp/src/FluidSolver.cpp:54:16: warning: unused variable ‘E’ [-Wunused-variable]
   54 |   const double E = 10000; // elasticity module
      |                ^

Other than that, it runs, the results look good (very similar with Python). I also tried all four combinations, and I get similar results.

Copy link
Copy Markdown
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, together with the unused variables, I noticed that the help mesages and the run scripts are out of date. @fsimonis please comment if you are working on this at the moment, otherwise I can fix it.

@fsimonis
Copy link
Copy Markdown
Member Author

@MakisH could you reimplement the command line arguments?
They are also completely lacking in the python solvers, but I guess they should be easy to add there.

@MakisH
Copy link
Copy Markdown
Member

MakisH commented Apr 14, 2021

I am currently going the other way around and removing them from the C++ version, as we have not really tested that this works with more values. We could, in the future, add them again.

Copy link
Copy Markdown
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the additional command-line arguments from the C++ version, as we have not really tested if arbitrary values work and I don't want to open another box at this point. It is anyway not important at all.

I also removed the Elasticity module from both the C++ and Python solvers, it was not needed there.

Ready to merge! 🎉

@MakisH MakisH merged commit 58f7497 into develop Apr 14, 2021
@MakisH MakisH deleted the rework-elastictube1d branch April 14, 2021 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants