[JOSS Review] Bits and Pieces - Version 2.2

[schuhmaj]

Changelog

Documentation

Have a look at the new deployed docs as temporary site
(This simplifies the review process. I'll remove that once the PR is merged.)

• Resolves [JOSS Review] Link to FORTRAN implementation is not working #19
  • The docs & the paper contain the link to the FORTRAN implementation with a reference when it was last accessed (still hope that the website may go online in the future again)
  • In the README.md, I fully removed the link.
• Resolves [JOSS Review] Fix Python syntax of minimal example #20
  • Adds trailing commas
• Resolves [JOSS Review] Minimal Python example doesn't run as expected #21
  • Fixes README.md examples
  • Fixes the examples in quick_python.rst
• Resolves [JOSS Review] Missing Ninja requirement for building from sources #22
  • The ninja dependency was hardcoded with an opt-out by setting the CMAKE_GENERATOR environment variable beforehand
  • This only affected the Python version when build from source with pip
  • Now, the build script uses the following precedences and does not fail if ninja is not found
    1. CMAKE_GENERATOR environment variable if set
    2. ninja if installed
    3. Any build system available on the system
• Resolves [JOSS Review] Improve tests by using densities different than 1.0 #27
  • Introduces a new parameterized test for the cube gravity with density 42
• Resolves [JOSS Review] Documentation missing statement of need. #30
  • Adds the Statement of Need to the documentation (this is basically the same paragraph as the one from the JOSS paper)
  • Update the citations of Sopot 2023 since DOIs are now available
• Updates Links to Documentation (in README.md, Jupyter Notebook, CONTRIBUTING.md) which now resides on GitHub Pages
• Resolves [JOSS Review] Minor improvement in paper paragraph #32
• Resolves [JOSS Review] Provide a list of dependencies needed to run the Jupyter Notebook #33
  • Adds an environment.yml for the Jupyter Notebook
  • Also provides a PyPi-based environment.yml for ARM-based systems
  • State the environment-setup inside the Jupyter Notebook
• Relates to Check validity of the polyhedra before computing gravity fields? #34
  • by providing a description of the Mesh Checking Utility in the README.md
• Update and Restructure the Documentation Overall!

Implementation

• Fix Accelerations sign
  • We now follow the geodetic and geophysics convention. Hence, the potential $V$ for a polyhedron with mass $m > 0$ is positive (as it was). However, the acceleration is now inverted according to $$ \textbf{g} = + \nabla V$$
  • Resolves [JOSS Review] Sign of acceleration components? #28
  • Resolves Sign of the tensor #29

TODOs when merged:

• Update Link to Documentation in Conda Recipe
• Delete temporary fork with the updated docs

---

Improvements based on openjournals/joss-reviews#6384

[schuhmaj]

@santisoler @mikegrudic

Thank you very much for the JOSS review and the detailed feedback on implementation, documentation, and paper 👍
This pull request should tackle the found issues; I look forward to your reply.

If there are still issues or something still needs to be sufficiently improved, please feel free to point them out.

---

@JoachimSchwabe

Also, many thanks to you for pointing out and detailing the sign issue of the accelerations 👍
This PR should fix this. Please Feel free to comment to point out issues if something is not yet good enough.

---

@gomezzz

You "must" review (approve) this PR so that it can be merged 🙃 Any comments are welcome.

[santisoler]

Thanks a lot for pushing all these changes @schuhmaj! I think this satisfies my requirements related to the JOSS review process.

I left one minor suggestion below, feel free to commit or edit it.

One thing I still noticed is I couldn't manage to install the Python package from sources with pip install . if Ninja is not installed. Even after the changes you made to setup.py in this branch. I don't think this is major issue for the JOSS review (I can actually install it with ninja-build installed in my system). But I wanted to raise the issue so you can keep investigating why this is happening. I can post some details in #22 if you desire.

Thanks again for these fixes. I enjoyed reviewing this submission.

[gomezzz]

Hi @schuhmaj !

Thanks for taking care of this 🙏 However, it is extremely difficult for me with all these changes and issues mixed in one PR to discern which change is supposed to do what and to resolve which issue.

From a quick glance, I suspect, the majority of the changes are doc changes and data changes?

If you want me to have a meaningful look I would ask you to maybe at least split it in

• code changes
• doc changes
• For data I would also suggest to use LFS or similar. Just looking at the txts almost crashes my browser :D

Otherwise I can only have a very cursory glance at the code as I have no idea how to connect intent of the change with the change 🙏

Sorry for being complicated :D

[schuhmaj]

@gomezzz Sorry for the confusion; I totally agree that it got crowded. In theory, it should be sufficient to review the three "actual code changes" and review the new docs (which are temporarily published under the given link to avoid the need to go through the all the *.rst files compared to the current docs).

(The incorporated change of santisoler is not included in the temporary build of the docs)

Code Changes

Actual Code Changes

• src/polyhedralGravity/calculation/GravityEvaluable.cpp:69 Multiply the acceleration with -1.0 to obey the geodetic convention
• test/calculation/GravityModelCubeTest.cpp Add a test case for the cube with density 42.0 in addition to the existing test case using density 1.0 using the parametrized construct from GoogleTest
• setup.py Remove the hard dependency from ninja as the build system and make it only the preferred solution if it is installed

Pseudo Code Changes

• Remove in every header file *.h docstring @param example - this is an example the - in between the parameter name and explanation to improve the automated documentation by doxygen/ sphinx
• The cube reference solution *.txt
  • Rename the old file to test/resources/analytic_cube_solution.txt to test/resources/analytic_cube_solution_density1.txt and multiply the accelerations by -1.0 to be coherent with the changes above
  • Add test/resources/analytic_cube_solution_density42.txt

Documentation ./docs/*.rst, README.md and CONTRIBUTING.md

This is the largest fraction of the PR. It is probably easier to review this temporary build.

To summarize the changes:

• State the convention (geodetic convention)
• Restructure and Optimize the Layout
• Add Statement of Need (only docs)

Paper

• [JOSS Review] Minor improvement in paper paragraph #32
• Link to GitHub Pages instead of ReadTheDocs
• The FORTRAN reference implementation's hosting site is down. The site might come back. Instead of a complete removal of the link, I added the date of last access, [JOSS Review] Link to FORTRAN implementation is not working #19

[gomezzz]

Thanks for elaborating! Okay, that actually makes it pretty tangible. My only remaining concern now is re the comment I left of introducing a non-breaking change that still fundamentally changes results. We can address it here or maybe it is better to merge this and have a separate issue + PR for it?

[gomezzz]

One thing I am mildly worried about is that once you push this if somebody built some code based on the old version and updates / reinstalls their env they will get wrong results without knowing about the change. It would be desirable to make their code break in that case. Maybe we could rename some function or something along the way to make this a breaking change that shows a message that the sign has changed if the they try to call the old version of this function in this release?

[schuhmaj]

Overall, I agree; however, I am unsure about how.
A warning or rename/ depcreation seems un-intuitive.

The best I could imagine would be something like a verbose option being True by default. The option could provide information about recent changes but also enable logging.
evaluate(..., verbose=False).
So, basically, a new feature.

On the other hand, simply having more options also seems a bit counterintuitive and contradicts the "slim, easy-to-use interface idea."

Not sure. Anyway, I will merge this so that we can get the JOSS paper rolling.

The verbose option might be something that fits nicely with #34, which will adapt the signature.
For now, I hope that a user who notices a different behavior's first idea is to check the Changelog—and there, I will definitely put the change in bold letters (Also, the package description now states which convention we follow).

[schuhmaj]

@gomezzz Or, alternatively, we increase the major version?

So, not 2.2, but rather 3.0. This is usually also an indicator of "breaking changes" and is much more recognizable.

[schuhmaj]

Thanks a lot for the review santisoler and gomezzz 🙂

Let's get this merged!