JOSS review comments

[guadabsb15]

Hello, these are some starting comments for review openjournals/joss-reviews#6805

For what I have seen so far, singularity-eos looks very helpful and comprehensive.

Looking at your Getting Started site in the documentation, I think it could be useful to have also -DSINGULARITY_BUILD_EXAMPLES=ON and then say something like "as a quick check of your installation go to /build/example and run ./get_sound_speed_press you should be able to see the following output: "

The final values are:
rho = 0.987467
uu  = 0.00987467
P   = 0.0059248
cs  = 0.0979796

I personally like having a sanity check when installing/using a new tool. And as a reviewer, is this the result I should be getting? I must admit I didn't compute the EOS values myself. I believe it is valuable to have the expected output of the example.

I'm not an expert in CMake, but I think it might look more consistent if the code snipets for both Getting Started and Examples used mkdir -p build && cd build, and not build for Example and bin for the Getting started?

For the Python Bindings documentation, it might be worth it to mention to add the singularity-eos\build\python to the $PYTHONPATH (or the path were users install singularity-eos /lib/python.../site-packages)

The EOS Models page doesn't show several of the Figures, is that on purpose?

I still need to check the written paper and a few more things, but these are my initial thoughts.

[Yurlungur]

Thanks @guadabsb15 for the suggestions! I just submitted an MR, #428 which should address these. Please take a look at your convenience.

[Yurlungur]

I personally like having a sanity check when installing/using a new tool. And as a reviewer, is this the result I should be getting? I must admit I didn't compute the EOS values myself. I believe it is valuable to have the expected output of the example.

I added the examples as a sanity check. But I think a more quantitative sanity check is the unit tests, so I added those to getting started as well.

The EOS Models page doesn't show several of the Figures, is that on purpose?

Not on purpose. It appears that whether or not the pdfs display properly is browser dependent. I changed the figures to pngs which should be supported by all browsers.

I'm not an expert in CMake, but I think it might look more consistent if the code snipets for both Getting Started and Examples used mkdir -p build && cd build, and not build for Example and bin for the Getting started?

Good point. I tried to make this more consistent.

For the Python Bindings documentation, it might be worth it to mention to add the singularity-eos\build\python to the $PYTHONPATH (or the path were users install singularity-eos /lib/python.../site-packages)

Good point. I added some verbage to this effect.

[guadabsb15]

Thank you for addressing my feedback, I will be taking a look at your PR hopefully tomorrow.

I read the software paper and it reads really well, I only have a question about one reference. In the performance portable polymorphism section, the citation for OpenMP Target Offload is from 2001. I could be wrong, but I thought OpenMP 4.0 was the first version that started supporting accelerators offload, and that was released in 2013? Or are you citing OpenMP in general?

[Yurlungur]

Thank you for addressing my feedback, I will be taking a look at your PR hopefully tomorrow.

Thanks @guadabsb15 ! Sounds good.

I read the software paper and it reads really well, I only have a question about one reference. In the performance portable polymorphism section, the citation for OpenMP Target Offload is from 2001. I could be wrong, but I thought OpenMP 4.0 was the first version that started supporting accelerators offload, and that was released in 2013? Or are you citing OpenMP in general?

Yes---the 2001 reference is the recommended citation for OpenMP as a whole. I will modify the citation location to clarify this. I'll also stick in a reference for OpenMP 4.0. I'll just sneak these into MR #428.

[guadabsb15]

Hi! I took a look at PR #428 and it looks good. However, for the quickstart I think the unit tests are a bit too involved/in depth. Just the example, to check the installation worked, like you put it is great.

[Yurlungur]

for the quickstart I think the unit tests are a bit too involved/in depth. Just the example, to check the installation worked, like you put it is great.

Fair enough! I removed the reference to the unit tests. Thanks @guadabsb15 . Should I go ahead and merge that MR and close this issue?

[guadabsb15]

@Yurlungur sounds good! The MR looks great and takes care of the comments in this issue.

[Yurlungur]

Great thank you!