ComputationalPhysiology / simcardems

Simula Cardiac ElectroMechanics Solver
http://computationalphysiology.github.io/simcardems
GNU Lesser General Public License v2.1
11 stars 11 forks source link

[JOSS Review] Performance and HPC #87

Closed mbarzegary closed 1 year ago

mbarzegary commented 1 year ago

Dear authors,

Since HPC is pointed out in the paper (by mentioning it as an advantage of the current implementation and leveraging FEniCS), I think it should be elaborated a bit in the documentation.

I couldn’t find anything regarding the parallel execution in the documentation. Are you relying on the user knowledge of FEniCS for this? If so, then the submission is useful for the FEniCS community only, limiting its potential to be used by broader users (I mentioned this as a problem for the installation documentation as well). I suggest that you explain how to run the code in parallel in the documentation. Also, what does the hpc argument do? It seems that it just turns off the progress bar, yes?

Furthermore, in my tests using 6 CPU cores (done using the demo example), the code runs slower when I call it with MPI. I guess that it may be related to the problem size, but anyway, it can be nice if you add another demo (maybe with a bigger problem size) for demonstrating the parallel execution and its benefits.

P.S. This issue is related to https://github.com/openjournals/joss-reviews/issues/4753

cdaversin commented 1 year ago

Thanks for your comment.

Indeed, the (former) hpc option was only meant to turn on and off the progress bar, since the progress bar display is not always properly handled when running on HPC clusters. We agree that the name of this option was misleading, and this has been changed into --show_progress_bar/--hide_progress_bar.

Running simulations in parallel doesn't require any changes in the code, since FEniCS handles the parallel processing. We have updated the documentation regarding parallel simulations with the command that should be used.

We don't expect good scalability with the demo example due to the problem size, as you mention. But we agree that it would be good to have a larger problem illustrating the parallel computing capability.