[REVIEW]: OGRe: An Object-Oriented General Relativity Package for Mathematica

whedon commented 3 years ago

Submitting author: @bshoshany (Barak Shoshany) Repository: https://github.com/bshoshany/OGRe Version: v1.6 Editor: @VivianePons Reviewer: @kostunin, @amelialdrew Archive: 10.5281/zenodo.5168868

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/068573253d55b405eb934bc915e72134"><img src="https://joss.theoj.org/papers/068573253d55b405eb934bc915e72134/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/068573253d55b405eb934bc915e72134/status.svg)](https://joss.theoj.org/papers/068573253d55b405eb934bc915e72134)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@kostunin & @amelialdrew, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @VivianePons know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @kostunin

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@bshoshany) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

Review checklist for @amelialdrew

Conflict of interest

[x] I confirm that I have read the JOSS conflict of interest (COI) policy and that: I have no COIs with reviewing this work or that any perceived COIs have been waived by JOSS for the purpose of this review.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Contribution and authorship: Has the submitting author (@bshoshany) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
[x] Substantial scholarly effort: Does this submission meet the scope eligibility described in the JOSS guidelines

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
[x] A statement of need: Does the paper have a section titled 'Statement of Need' that clearly states what problems the software is designed to solve and who the target audience is?
[x] State of the field: Do the authors describe how this software compares to other commonly-used packages?
[x] Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @kostunin, @amelialdrew it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf

whedon commented 3 years ago

Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.02 s (153.3 files/s, 137489.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Mathematica                      1            155            487           1852
Markdown                         2             34              0            162
-------------------------------------------------------------------------------
SUM:                             3            189            487           2014
-------------------------------------------------------------------------------

Statistical information for the repository 'b743ec218688700898811fdc' was
gathered on 2021/06/25.
No commited files with the specified extensions were found.

whedon commented 3 years ago

PDF failed to compile for issue #3416 with the following error:

 Can't find any papers to compile :-(

bshoshany commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago

Attempting PDF compilation from custom branch paper. Reticulating splines etc...

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

whedon commented 3 years ago

:wave: @amelialdrew, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

:wave: @kostunin, please update us on how your review is going (this is an automated reminder).

kostunin commented 3 years ago

busy with conference, will come back to review soon, sorry for delay

VivianePons commented 3 years ago

Dear @amelialdrew and @kostunin , have you had time to start looking at the paper?

kostunin commented 3 years ago

Hi @VivianePons my conference is just finished today, I will have a look next week, sorry for delay again.

amelialdrew commented 3 years ago

Hi @VivianePons apologies I have also been very busy but will start next week

VivianePons commented 3 years ago

No problem! Just wanted to make sure it was still going on. I'll be taking a summer break myself by the end of the week so no rush

amelialdrew commented 3 years ago

Hi @VivianePons, what is the best channel to suggest small changes/additions?

VivianePons commented 3 years ago

You can discuss them here or directly on the software repo, that's up to you

amelialdrew commented 3 years ago

Hi @bshoshany, this is looking good to me so far, it seems as though it would be useful for students getting to grips with Mathematica/GR and the documentation is very clear. I have a few small questions/comments organised roughly into the checkpoints I haven’t ticked off yet:

Do you have any examples of where the code has been cited/used for more advanced research projects?
Main parallelisation section for me only speeds up by 30-40% and I get an error when running TSetParallelization[True]:

Parallelization enabled.
Part: Part 1 of {} does not exist

Other performance/optimisation claims throughout e.g. ‘Built with speed and performance in mind using optimized algorithms’ are not backed up with numbers, but this may not be necessary.

Target audience is in Summary rather than Statement of Need, I expect this is fine.
I think it would be worth citing the references throughout rather than just the list at the end. I would also suggest including a review of other GR/tensor manipulation codes, e.g. https://link.springer.com/article/10.1007/s41114-018-0015-6. Also, the link for the first review on the paper doesn’t work.

Willing to discuss all comments as I am not yet an experienced reviewer.

bshoshany commented 3 years ago

Hi @amelialdrew,

Thanks so much for reviewing my package and for your very helpful comments! Here are my replies:

Unfortunately I do not have any examples of this code being used in research yet, except for my own research. The package is only a few months old and it will probably take some time before people who are using it produce any papers with it. However, it is currently being used by both myself and 4 of my students, and I hope to publish the first paper citing the package in a couple of months.
How many cores does your system have? The speedup in the documentation is with 12 cores and 8 kernels. If you have less than that, then the speedup will be lower.
When TSetParallelization is turned on, it tries to start all available kernels, but executing LaunchKernels[] would fail if some kernels are already running, so it first determines how many kernels are already running and then starts only those that are not. You may have found a bug. When you execute $ConfiguredKernels, what do you see? It should be something like {<<N local kernels>>} and $ConfiguredKernels[[1, 1]] should return the total number of kernels available. From your error message I suspect $ConfiguredKernels does not return a List on your system. Which version of Mathematica are you using?
The speedup from the parallelization is probably the only one that can be directly measured. But when I say I built it with speed and performance in mind, I am also referring to many other things happening behind the scene. For example:
1. The components of the tensor in any particular representation are not calculated until they are needed. Once they are calculated, they are stored permanently within the tensor object so they don't need to be calculated again later.
2. If you're trying to calculate e.g. the Ricci tensor, then OGRe first checks if the Riemann tensor has already been calculated. If so, then it uses the result of that calculation when calculating the Ricci tensor, instead of calculating it from scratch. And similarly for all other curvature tensors.
3. The ability to export and import tensor objects means that in your next Mathematica session you can import the tensors with everything that has been calculated so far (components in different representations, curvature tensors, etc.) instead of having to calculate everything from scratch.
4. And so on.
If you want, I'm happy to move the target audience from the Summary to the Statement of Need.
The references I linked to are just general references for differential geometry and general relativity that don't necessarily apply to any specific word or sentence, but to the paper as a whole. However, I can convert them to inline references if you prefer.
I'm not sure why the ISBN link to the Baez and Munian book isn't working. The ISBN is correct but the link itself was generated by JOSS. I'll replace it with a different ISBN (9789810217297) for which the link does seem to work.
I'll add a reference to the paper by MacCallum, which indeed seems relevant.

Let me know which of the above changes you want me to make, and then I'll upload a new version of the paper!

VivianePons commented 3 years ago

/ooo July 30 until August 22

VivianePons commented 3 years ago

Dear all, I'm glad to see that the review is going on. I'll be on leave for the next 3 weeks. We'll see where we're at when I come back! Have a nice summer

bshoshany commented 3 years ago

Have a great summer @VivianePons!

amelialdrew commented 3 years ago

Hi @bshoshany,

Thank you for the quick reply and for the further information regarding parallelisation. My system has 4 cores which partially explains the slower speedup, but I would say not entirely. Running $ConfiguredKernels I get just {}, so it seems you are right that it is not returning a List. I am using Mathematica 12.3.1. Do you need any further info to help with the bug?

Further to sorting out this parallelisation issue, I would recommend:

Moving target audience to Statement of Need (for no other reason than it says that specifically on our checklist!)
Converting some of the more specific references to inline e.g. xACT package, adding the review paper as discussed and fixing the link.

Let me know what you think!

amelialdrew commented 3 years ago

@VivianePons Happy holidays :)

bshoshany commented 3 years ago

@amelialdrew, are you running the code on the Wolfram Cloud, by any chance? That might explain why $ConfiguredKernels returns {}. If not, how many kernels does your Mathematica license allow? This can be found under Preferences -> Local Kernels. You will see an option "Limit by license availability (N)" where N is the number of kernels available in your license. And which OS are you using?

In any case, I will fix this bug by simply not launching any kernels in advance if $ConfiguredKernels is empty. The kernels will then instead be launched automatically by Mathematica the first time a parallelized task is executed. I will incorporate this fix into the next release, which should be ready within a week, and will also include some cool new features I've been working on, such as calculating the geodesic equation, more compact display of tensor components using TList and TShow, and more.

I will make the changes to paper.md as you requested, and upload it to the paper branch when I'm done. I will take that opportunity to also add the additional features from the upcoming v1.6 release to the list of features in the paper. I'll do that once the new release is up.

amelialdrew commented 3 years ago

@bshoshany No I am not, although I've just looked under Preferences and it says Local Kernels is deprecated, so I assume this more up to date version of Mathematica has removed them. I am using macOS Big Sur 11.4. The updates sound good!

bshoshany commented 3 years ago

Hmm... I also use v12.3.1, and I also see the message "Local Kernels is deprecated", but I also see the number of local kernels in the preferences, and my $ConfiguredKernels returns a List. I get that on both Windows and Linux (I don't have access to a Mac).

The documentation for $ConfiguredKernels says that "As of Version 12.3, $ConfiguredKernels has been superseded by $DefaultKernels", but since I want the package to be compatible with Mathematica all the way down to version 12.0 (some people do not upgrade because that costs extra), I can't use that.

However, I found a simple solution; CloseKernels[]; LaunchKernels[] should launch all the available kernels regardless of how many are already running, so I'll just do that instead of what I do now for TSetParallelization.

I can't find any information in the release notes or documentation about the Local Kernels themselves being deprecated. Perhaps there is some new parallelization framework that doesn't use multiple local kernels, but I can find any evidence of that. I might have to contact Wolfram directly to ask what the deprecation means.

I have a few more questions to understand the bug better, if you don't mind:

What do yet get when you execute $MaxLicenseSubprocesses?
Start a fresh Mathematica session (after closing all the currently open Mathematica windows if any) and type LaunchKernels[]. The output should be a list of kernels. How many kernels are in the list?

Thanks!

amelialdrew commented 3 years ago

Ah I've just realised there's an option for Enable Local Kernels which by default runs with 4 kernels with 4 CPUs which approximately halves the time vs no parallelisation, so this may fix it. I suppose it would be worth adding a line in the docs to make sure they are enabled.

In case it is still needed, $MaxLicenseSubprocesses gives me 16, and LaunchKernels[] gives 4 kernels, both after switching on Enable Local Kernels.

I hope this helps!

bshoshany commented 3 years ago

@whedon generate pdf from branch paper

whedon commented 3 years ago

Attempting PDF compilation from custom branch paper. Reticulating splines etc...

whedon commented 3 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

bshoshany commented 3 years ago

@amelialdrew: The new release is now up, along with the modifications you requested to the paper in the paper branch. I believe I solved the parallelization bug, so please check and let me know if it works now or if you still get an error.

I am happy to answer any additional questions and make additional modifications as needed.

amelialdrew commented 3 years ago

Hello @bshoshany, thanks for the new changes. The new version works and for me gives 16 parallel kernels launched. CPU has 4 cores which is more than the default, as I think you intended. It still only speeds up from ~13s to ~8s, but I would guess this is system/version dependent, and as it does speed up I am happy to approve.

bshoshany commented 3 years ago

Hi @amelialdrew, I do believe the speedup you get is exactly what I would expect you to get.

The number that counts is the number of cores, since that is the number of things the CPU can actually do in parallel. On my computer I have 12 cores but only 8 kernels due to license limitations, which means 4 of my 12 CPU cores are not actually being utilized. With the 8 kernels running in parallel on 8 cores, I get a speedup from ~10s to ~3.5s, which is roughly a 2.9x speedup. (One might expect to get an 8x speedup, but Mathematica's parallelization is very inefficient.)

On your computer your license allows 16 kernels, but that doesn't mean much, because you only have 4 cores, so only 4 of these kernels are actually running in parallel at any given time. Therefore the number of actual parallel kernels you have (4) is half the number of actual parallel kernels I have (8). You get a 1.6x speedup, which is roughly half the speedup I get, so that's perfectly consistent.

Thank you so much for completing the review! 🙂

amelialdrew commented 3 years ago

Great, thank you!

VivianePons commented 3 years ago

Hi there,

from what I gather, @amelialdrew it looks like you are done with the review and happy with the paper. Thank yo very much for your time and expertise.

@kostunin do you think you can have a look soon? Thanks

kostunin commented 3 years ago

yes, will start today, apologize once again for the delay

kostunin commented 3 years ago

@VivianePons I have started the process, but cannot tick checkboxes, is it correct?

@bshoshany following requirements in section "Substantial scholarly effort": could you please provide some papers used this software like stated in "Whether the software has already been cited in academic papers." Might be useful to add them to github wiki of your project.

I guess the requirements in "Whether the software is sufficiently useful that it is likely to be cited by your peer group." is fulfilled as well as others in "Substantial scholarly effort", only the exemplary papers are missed.

danielskatz commented 3 years ago

@whedon re-invite @kostunin as reviewer

whedon commented 3 years ago

The reviewer already has a pending invite.

@kostunin please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

kostunin commented 3 years ago

The reviewer already has a pending invite.

@kostunin please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

Sorry, we couldn't find that repository invitation. It is possible that the invitation was revoked or that you are not logged into the invited account. :( -- from the github

danielskatz commented 3 years ago

@whedon re-invite @kostunin as reviewer

Trying again

whedon commented 3 years ago

OK, the reviewer has been re-invited.

@kostunin please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

kostunin commented 3 years ago

@danielskatz works, thanks! @bshoshany I saw comment on my question regarding papers. Let me specify the question: can you provide any papers/slides with usage of this code or prototype just in this thread although the published version is not cited? I guess by this we can fulfill official requirements

kostunin commented 3 years ago

@bshoshany I did not find any automated tests for the software. I suggest they can be implemented as verification of some (obvious) calculations given in main documentation with explicit conditions, e.g. TCovariantD["μ"]."FLRWEinstein"["μν"] == ... and these tests should be put in some separate notebook I assume (not sure how automatic tests work in Mathematica, sorry)

kostunin commented 3 years ago

@bshoshany I have also issue on parallelization: evaluation hangs after cell 198 (documentation notebook). I have no idea whether it is related to ClearSystemCache or to something else.

Regarding paper text: I suggest to put bit more attention on GR in summary, since this topic is mentioned in the title, but almost nothing said in text itself (might be a bit misleading, but I am not a big expert in this field though).

The final conclusion: the paper is recommended for the publication after implementing minor comments/fixes mentioned above

VivianePons commented 3 years ago

@whedon re-invite @kostunin as reviewer

whedon commented 3 years ago

@kostunin already has access.

VivianePons commented 3 years ago

@kostunin I have made whedon send you a new invite which should now allow you to tick the boxes.

kostunin commented 3 years ago

thanks @VivianePons, I have reviewed the paper and software and the overall feedback is positive, the few minor items have to be completed before publication.

VivianePons commented 3 years ago

Great, thank you very much for your work! I let @bshoshany look at your remarks and work on the minor issues

bshoshany commented 3 years ago

Hi @kostunin,

Thank you so much for your review! :) Here are my replies to your comments.

Regarding papers using the package:

The package is fairly new and it has not yet been cited in any academic papers to my knowledge. In fact, one of the main reasons for publishing it on JOSS is so that more people will be aware of its existence!

My students and I use it all the time in our research, and I am planning to publish several different papers where the package was extensively used to produce results - possibly even with Mathematica notebooks attached. However, for obvious reasons, I would prefer not to share any such work in progress.

The guidelines say "Whether the software is sufficiently useful that it is likely to be cited by your peer group". Clearly the package works, does very useful things, and there are numerous examples in the documentation of how it can be used in research (e.g. to calculate curvature tensors and geodesics, perform arbitrary tensor calculations, and so on). Would you consider that to be sufficient?

If not, I am willing to get on a Zoom call with you and show you some of the Mathematica notebooks used by me and my students for our research. But I would rather not share the files themselves, publicly or privately, until the papers have been published (or at least uploaded to the arXiv).

Regarding tests:

Writing comprehensive automated tests is something that I certainly plan to do in the future, but it's definitely not an easy thing to do, and will take a long time to complete - at least a few months, given that the academic year is about to start and I'm teaching 4 courses so I won't have much time for programming...

What I have been doing so far in order to check that everything works whenever I release a new version is to evaluate the documentation notebook, go over it, and manually make sure everything is in order. Since the documentation covers every single feature of the package, I believe this is a sufficiently comprehensive test, although not automated.

The review checklist says "automated tests or manual steps described so that the functionality of the software can be verified", so automated tests are not mandatory, and I believe the manual test using the documentation notebook, as described above, is sufficient to verify the functionality of the software. What do you think?

Regarding parallelization:

Other than Amelia's issue described above, which stemmed from using a deprecated Mathematica feature in an older version of OGRe, I have never encountered any issues with parallelization, nor have my students or any other users encountered any such issues to my knowledge. If evolution hangs, that might be due to insufficient memory or CPU power on your system.

Can you please do the following:

Provide more information about your system, including details about your CPU, memory, OS, Mathematica version, number of kernels allowed in your license, and anything else that might be relevant?
Try to reproduce the issue without executing the entire documentation notebook first, by copying the relevant cells to another notebook and executing them in a fresh Mathematica session?
Try to reproduce the issue by performing a different parallelized operation: open a new Mathematica notebook, load OGRe, enable parallelization, create some complicated metric, and then try to calculate its curvature tensors and see if that works?

If you wish, I am happy to have a Zoom call sometime to get to the bottom of this! Another option would be to move this to an issue in the package repository and try to resolve it separately from this review.

Regarding more GR in summary:

I suggest replacing the first paragraph with the following.

OGRe is a modern Mathematica package for differential geometry and tensor calculus. It can be used in a variety of contexts where tensor calculations are needed, in both mathematics and physics, but it is especially suitable for general relativity - the field of physics where tensors are most commonly and ubiquitously used. Whether the user is doing cutting-edge research in general relativity or just making first steps in learning the theory, the ability to manipulate tensors and perform tensor calculations quickly, easily, and intuitively will greatly simplify and accelerate their work.

Please let me know what you think, and if this is okay, I will update paper.md!

openjournals / joss-reviews