[JOSS Review] Reproducibility and functionality

mbarzegary commented 1 year ago

Dear authors,

Generally speaking, the software runs as described in the documentation, and the demos can be set up by following the instruction. However, there are minor fixes that can improve the quality of the submission:

The about page is empty, which is the first thing the user sees when the GUI is loaded (it says “The SIMula CARDiac ElectroMechanics solver is ...”)
After running the simulations, I couldn’t visualize the results in ParaView. The XDMF files are there but the XDMFReader cannot open them. I tried to open them using PV 5.8 and 5.9 in Windows and PV 5.7 in Linux, always got a “cannot load data” error. You may need to elaborate on the postprocessing steps in the documentation.
It’s unclear what “node_A”, “node_B”, etc., means in the postprocessing section of the GUI. Which nodes are they referring to? This may be addressed in the documentation as well, as mentioned above.
While checking the demo for the command-line interface, I noticed that python3 -m simcardems run --end-time=100 works but python3 -m simcardems run -T=100 doesn’t. It says “Error: Invalid value for '-T' / '--end-time': '=100' is not a valid float.”

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

IlsevanHerck commented 1 year ago

Thank you for the comments @mbarzegary.

I just have 2 short questions regarding the ParaView visualisation:

How did you open the xdmf-files? Via the 'load data' option in ParaView, open ParaView from the xdmf file, or another way?
Does the xdmf file have a corresponding .h5 file in the same folder?

mbarzegary commented 1 year ago

hi, 1) I open them in PV the way I do for fenics results, like by "File->Open" or by dropping the the xdmf file on the pipeline browser. 2) yes, the files have corresponding h5 files beside. I share the results I got for demo example for your reference if it helps: https://we.tl/t-ZrxRmMlJsB. the link expires in 7 days.

IlsevanHerck commented 1 year ago

Thanks @mbarzegary ! We're looking into this issue and will get back to you later.

jorgensd commented 1 year ago

It seems to be an issue with boost and string concatenation when generating the XDMFFile on your system. This has been fixed in DOLFINx: https://github.com/FEniCS/dolfinx/pull/1743, https://github.com/FEniCS/dolfinx/pull/1826 but has not been patched in DOLFIN. For instance: https://bitbucket.org/fenics-project/dolfin/src/74a6ac2095059ea6582fe4ea11686c1c723e14f5/dolfin/io/XDMFFile.cpp#lines-1822 would have to be changed.

jorgensd commented 1 year ago

Fix proposed in: https://bitbucket.org/fenics-project/dolfin/pull-requests/555/fix-xdmffile-string-merging-with-h5

jorgensd commented 1 year ago

@mbarzegary This has now been patched in conda: https://github.com/conda-forge/fenics-feedstock/pull/169#issuecomment-1313729111 Did you use conda when you got this issue, or did you install from source?

mbarzegary commented 1 year ago

Great! Yes, I installed FEniCS inside a conda environment.

jorgensd commented 1 year ago

Great! Yes, I installed FEniCS inside a conda environment.

Then it should hopefully work now with the latest conda builds: https://anaconda.org/conda-forge/fenics/files

mbarzegary commented 1 year ago

Yes, I checked it, and it works fine now.

finsberg commented 1 year ago

We have now addressed all of these issues. Feel free to re-open this if needed.

ComputationalPhysiology / simcardems

[JOSS Review] Reproducibility and functionality #85