Pooltool Submission

Question

Pooltool Submission

ekiefl opened this issue 2 years ago · 43 comments

Answer 1 · 2024-04-16T17:18:42.000Z

Editor in Chief checks

Hi there! Thank you for submitting your package for pyOpenSci
review. Below are the basic checks that your package needs to pass
to begin our review. If some of these are missing, we will ask you
to work on them before the review process begins.

Please check our Python packaging guide for more information on the elements
below.

Initial onboarding survey was filled out
We appreciate each maintainer of the package filling out this survey individually. 🙌
Thank you authors in advance for setting aside five to ten minutes to do this. It truly helps our organization. 🙌

Editor comments

paper.md not yet merged, but this is not blocking https://github.com/ekiefl/pooltool/pull/122/files

Answer 2 · 2024-04-19T05:16:08.000Z

Hi @Batalex, I've made the following changes:

I've merged paper.md
I made a new release (v0.3.2)
I made a DOI for v0.3.2: https://doi.org/10.5281/zenodo.10995500

I edited the first post with the bumped version (v0.3.2) and the DOI.

Regarding the survey: I already filled this out, but if your records show otherwise, I'm happy to do it again.

With this all finished, I think we're done with pre-review checks. Please let me know if there's anything else left to do.

Thanks for your time.

Answer 3 · 2024-05-10T05:50:52.000Z

In anticipating pooltool being reviewed in pyOpenSci, I've given a lot of thought about my licensing and I've decided to switch licenses from GPLv3 to Apache v2. I figured it would be better to do this before any reviews start, so I've made the following changes:

Merged the license change (ekiefl/pooltool#123)
Made a new release (v0.3.3)
I made a DOI for v0.3.3: https://doi.org/10.5281/zenodo.11171124

I edited the first post with the bumped version (v0.3.3) and the DOI.

Answer 4 · 2024-05-20T08:55:54.000Z

Hey @ekiefl,
I am very pleased to announce that @cmarmo will oversee leading the review as the editor! Chiara will be your first line responder from now on, though you are welcome to ask anyone if you have any question during the process.
Happy review!

Answer 5 · 2024-05-20T20:44:54.000Z

Hi @ekiefl , nice to meet you, and thanks for submitting to pyOpenSci!
I'm Chiara and I'm going to take care of the process as editor.
I'm looking for reviewers right now and hope to be back to you by the end of the week.

Answer 6 · 2024-05-23T16:53:11.000Z

@Batalex, thanks for your help thus far.

@cmarmo, thanks so much for continuing the effort :)

Please let me know if I can help in any way.

Answer 7 · 2024-05-28T12:31:22.000Z

Hi @ekiefl! I just want to let you know that I had some promising answers from possible reviewers: they asked some more time to evaluate the engagement.
Finger crossed, I'll be back in a week with good news.... 🤞

Answer 8 · 2024-05-30T16:11:33.000Z

That's fantastic! Fingers crossed

Answer 9 · 2024-06-06T18:47:35.000Z

Sorry @ekiefl bad luck for this first round of seeking for reviewers .... I'm following some new tracks ....

Answer 10 · 2024-06-06T22:55:33.000Z

Too bad! Ok sounds good. Thanks for all your effort!

Answer 11 · 2024-06-24T10:19:48.000Z

Hi @ekiefl, I am happy to share that I've found our first reviewer for Pooltool.
@sebastianotronto kindly accepted to review pooltool for pyOpenSci: thank you very much Sebastiano! 🙏
Feel free to introduce yourself here , while I'm pursuing my quest for a second reviewer... 🙂

Answer 12 · 2024-06-24T17:32:49.000Z

Hi everyone!

I am Sebastiano, nice to meet you. I work as a scientific software developer in the private sector, but less than two years ago I defended my PhD in pure Math (number theory). I code mainly in C# (for work) and C (for fun), but I have some experience with Python too.

This is going to be my first review for pyOpenSci.

Answer 13 · 2024-06-25T01:36:07.000Z

Hey @sebastianotronto, thank you so much for agreeing to be a reviewer! I also defended my PhD a couple of years ago, not in pool (unfortunately) but in biophysics 😃

Answer 14 · 2024-07-08T21:10:28.000Z

Hello everybody, I'm waiting for some answers from potential second reviewers.
In general we try to have both reviewers working at the same time, but in order to move forward, and if @sebastianotronto is ready, perhaps we can start with the first review: @ekiefl would this be ok with you?

Thank you very much for your understanding 🙏

Answer 15 · 2024-07-08T21:39:05.000Z

@cmarmo, first of all let me just say thank you for your continued support in trying to find reviewers. I really appreciate your dedication to open scientific software 🙏

@sebastianotronto please feel free to start your review whenever, and thanks again for agreeing to review. Let's hope you're a trendsetter and we get a second reviewer soon enough.

Answer 16 · 2024-07-09T07:54:02.000Z

Sure, here is my review! Apologies if I misunderstood some requirements from the standard template.

Package Review

As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

A statement of need clearly stating problems the software is designed to solve and its target audience in README.
Installation instructions: for the development version of the package and any non-standard dependencies in README.
Vignette(s) demonstrating major functionality that runs successfully locally.
Function Documentation: for all user-facing functions.
Examples for all user-facing functions.
Community guidelines including contribution guidelines in the README or CONTRIBUTING.
Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.

Readme file requirements
The package meets the readme requirements below:

Package has a README.md file in the root directory.

The README should include, from top to bottom:

The package name
Badges for:
- Continuous integration and test coverage,
- Docs building (if you have a documentation website),
- A repostatus.org badge,
- Python versions supported,
- Current package version (on PyPI / Conda).

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Short description of package goals.
Package installation instructions
Any additional setup required to use the package (authentication tokens, etc.)
Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
- Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
Link to your documentation website.
If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
Citation information

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Package structure should follow general community best-practices. In general please consider whether:

Package documentation is clear and easy to find and use.
The need for the package is clear
All functions have documentation and associated examples for use
The package is easy to install

Functionality

For packages also submitting to JOSS

The package has an obvious research application according to JOSS's definition in their submission requirements.

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

A short summary describing the high-level functionality of the software
Authors: A list of authors with their affiliations
A statement of need clearly stating problems the software is designed to solve and its target audience.
References: With DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 4.5

Review Comments

Here are my general comments. In bold the parts that require an answer or action.

Installation

I had minor hiccups during the installation because the tool relies on panda3d version 11 (not released yet). The installation instructions are clear, but I tried using an unsupported python version first (3.12) and that did not work out. It would be nice if the tool could be installed without workaround for panda3d, and possibly on the latest python version. Is it just a matter of waiting for panda3d v11 to be released?

Graphical mode

The tool also offers a graphical mode that can be "played" like a 3D game. The overall effect is very realistic and it appears clear that the physics is accurately simulated. I also liked the mouse + keyboard controls, personally I found them intuitive.

The author does not make specific claims about performance, but the graphical mode was quit demanding on my laptop, with extreme lag. On the other hand, it worked like a charm on my much more powerful desktop.

On my laptop I also encountered some graphical glitches (see pictures below):

Help menu (but also other text) is cut out when playing with unusual resolution (I was keeping two windows side by side)
Stick disappeared after entering visual mode, only reappears in shot mode or English mode.

A couple of times I started a shot that took a very long time to calculate, i.e. more than 10 seconds on my desktop. Both times this happened at the very first shot of the game, default mode. I suspect this is because the starting position of many balls all close together generates a huge number of events compared to other shots (the author explained in his blog that the simulation algorithm is event-based). I am curious if the author is aware of this and if there are plans to improve the sometimes slow first shot.

Documentation and API

The documentation is clear and concise and it makes it easy to get started with the package. It is very much still in the making and some parts are marked as incomplete, but the essentials are there and well written.

The documentation contain an "Hello World" example script to use the APIs, and an in-depth explanation of this example. Very nice way to get started.

The API documentation is automatically generated and it contains plenty of examples. As I noted above, not all functions have examples, but considering how fine-grained the API is I do not think it would be necessary.

I also enjoyed reading the author's blog posts where he explains the physics of the problem and the algorithm used for the simulator. Although not part of the official documentation of the package, it is definitely a nice addition.

Code

The code is generally high-quality and easy to understand, despite the size of the package.

Possible performance improvement

I have also been thinking about some possible performance improvement in the main simulation loop. I don't know if my suggestion would actually lead to any significant improvement, but it is interesting to think about it.

Let's consider the simplified case where there are n balls and the only possible events are ball-ball collisions. Right now the main simulation loop would compute, at every iteration, O(n^2) possible collisions. I am wondering if it is possible to not re-do every collision check at each iteration, saving some work.

Say for example that at the first iteration it was found that the next collision is between ball 1 and ball 2. At the second iteration, instead of computing all O(n^2) ball-ball collisions, we could compute just the O(n) collisions involving ball 1 or ball 2. Indeed any two other balls, say for example ball 3 and ball 4, are not affected (yet) by the collision of ball 1 and ball 2, so their next collision time will remain the same computed in the first iteration.

This could cut down significantly the number of computations done at each iteration.

Answer 17 · 2024-07-13T20:58:06.000Z

@sebastianotronto, I wanted to respond sooner rather than later to let you know that I've read your review, and I'm super appreciative of how thorough it is.

These are great suggestions and I'm going to start making my way through your comments to try and improve some of the areas you've pointed out.

Answer 18 · 2024-07-15T20:38:43.000Z

Dear @ekiefl, I am really happy to announce that we have our second reviewer. 🥳
@eliotwrobson kindly accepted to help us as a reviewer: thank you so much! 🙏

As summer is striking in the Northern hemisphere (which is essentially the one involved here), let's say that it would be wonderful to have the review submitted before August 5th. @eliotwrobson, is that ok with you? Thanks for your help!

Answer 19 · 2024-07-16T02:21:41.000Z

@cmarmo I think August 5 sounds reasonable! I'll let you know if anything comes up. My schedule right now is a little in flux but I should be able to make that.

EDIT: looking at the previous review, it seems like there has been significant discussion of usability and the GUI already. I'm going to focus my review on performance, since that was highlighted as an issue, and try to give more concrete suggestions there.

Answer 20 · 2024-07-17T04:16:21.000Z

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

A statement of need clearly stating problems the software is designed to solve and its target audience in README.
Installation instructions: for the development version of the package and any non-standard dependencies in README.
Vignette(s) demonstrating major functionality that runs successfully locally.
Function Documentation: for all user-facing functions.
Examples for all user-facing functions.
Community guidelines including contribution guidelines in the README or CONTRIBUTING.
Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.

Readme file requirements
The package meets the readme requirements below:

Package has a README.md file in the root directory.

The README should include, from top to bottom:

The package name
Badges for:
- Continuous integration and test coverage,
- Docs building (if you have a documentation website),
- A repostatus.org badge,
- Python versions supported,
- Current package version (on PyPI / Conda).

NOTE: If the README has many more badges, you might want to consider using a table for badges: see this example. Such a table should be more wide than high. (Note that the a badge for pyOpenSci peer-review will be provided upon acceptance.)

Short description of package goals.
Package installation instructions
Any additional setup required to use the package (authentication tokens, etc.)
Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.
- Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)
Link to your documentation website.
If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.
Citation information

Usability

Reviewers are encouraged to submit suggestions (or pull requests) that will improve the usability of the package as a whole.
Package structure should follow general community best-practices. In general please consider whether:

Package documentation is clear and easy to find and use.
The need for the package is clear
All functions have documentation and associated examples for use
The package is easy to install

Functionality

For packages also submitting to JOSS

The package has an obvious research application according to JOSS's definition in their submission requirements.

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

A short summary describing the high-level functionality of the software
Authors: A list of authors with their affiliations
A statement of need clearly stating problems the software is designed to solve and its target audience.
References: With DOIs for all those that have one (e.g. papers, datasets, software).

Final approval (post-review)

The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing:

Review Comments

It was hard to find the package installation instructions (as there wasn't a quick-start guide on the README), and I had to scroll to the docs section of the README. Also, there is no README on pypi, so it made it hard to tell that this was associated with this package.
It would be nice to have fewer steps to install the dev version. I see that poetry is being added in a PR, so that should alleviate this issue.
For a package this size, it would be nice to have all of the tests in a separate folder.
The pyproject.toml is missing metadata (like author email), but that looks like it will be fixed once the poetry PR is merged.
Some badges are missing. In addition to the ones on the checklist, you should have a badge for the discord there too so people can find it right away.
The API documentation seems very complete, but the formatting is a little awkward and hard to read. For example, this indentation looks strange:

Since there are many large functions, it might be easier to switch to giving each function it's own page, the way that the numpy docs are set up. More math in the docs would be good.

3.12 support would be good, along with test coverage. It looks like your CI only runs 3.8, you should definitely expand to different versions and control versions of dependencies somehow. 3.8 Is going end of life pretty soon.
I think there needs to be a couple more vignettes. It would be awesome to have a few that show tasks that could arise in some type of research.
I know that the type annotations aren't read by numba and that library has it's own type conversions, but it would be nice to have them on the numba jit-ed functions to make them easier to read.

Answer 21 · 2024-07-17T04:28:38.000Z

@ekiefl I've just started poking around the package, looks cool so far! I was having trouble getting the GUI to work on my machine, do you happen to have code that produces the slowdown that @/sebastianotronto pointed out? I'd like to do some profiling.

Answer 22 · 2024-07-17T05:25:35.000Z

@cmarmo thank you so much for your exhaustive search!

And @eliotwrobson, I really appreciate you agreeing to review. Thank you!

I was having trouble getting the GUI to work on my machine

That's a bit troubling. What are your specs?

do you happen to have code that produces the slowdown that @/sebastianotronto pointed out?

@sebastianotronto I will respond to your review more fully soon, but since @eliotwrobson is bringing up the performance hits that often occur (several seconds or more), I figured it makes sense to provide some further information for both of you.

While it's definitely possible and probably practical to speed up the algorithm via your suggestion of caching event times to avoid recalculation, I believe the large slowdowns you sometimes observe have to with the package numba.

Numba offers just-in-time compiling of python code into LLVM machine code, which offers performance speeds similar to what one would expect with a low level language like C or Rust or something. And pooltool uses numba extensively in its shot evolution algorithm (probably in around 20+ functions). This is great, but since these functions are just-in-time compiled, the first call is always slow because the function is compiled on the spot. Subsequent calls during that python process are fast because the function is compiled.

Fortunately, numba offers the ability to cache the compiled functions, so that a function compiled on a Tuesday can be re-used on a Wednesday. Pooltool makes full use of this feature to try and ameliorate the problem of re-compiling every time someone boots up a pooltool script. This saves a ton of time, however there is still a significant overhead just to load the cached code.

TLDR: numba can cause the first shot to be slow either because the numba functions needs to compiled (really slow) or they've previously been compiled but they still need to be loaded from cache (just kind of slow). Pile on the fact that the break shot is intrinsically slower because there are more events to calculate, and we can end up with some long wait times.

Partial solution: While numba compilation and cache loading is inevitable, I think the UX could be improved by calculating an "offscreen" shot as soon as the GUI is opened and introducing a "please wait" screen while the numba's housekeeping business is occurring.

@eliotwrobson, now that you have more context, if you are still interested in profiling the code, you might try poking around in sandbox/break.py for inspiration. For example, this will calculate 20 breaks and calculate the average time taken for the shot evolution algorithm:

python sandbox/break.py --no-viz --time-it

If you like profiling with cachegrind files, you can create a cachegrind of a break shot with

python sandbox/break.py --no-viz --profile-it

In the script you can see lines where I "burn" a break shot to avoid numba compilation/cache loading from being included in the timing/profiling:

        # Burn a run (numba cache loading)
        pt.simulate(shot, continuous=True)

Answer 23 · 2024-07-17T18:06:54.000Z

@ekiefl thanks for the explanation! Your proposed solutions sounds very reasonable.

Answer 24 · 2024-07-18T03:28:33.000Z

@ekiefl Thank you for your fast response! I ran the command you sent and I think you're right. Even on my system, the output of the benchmarks came out to fractions of a second, so most of the time probably is just cache retrieval. The flamegraph I got is below:

I was able to get this during a cached run, and it seems to confirm the discussion on the blog, that most of the time is spent computing all of the events that can happen and finding the minimum. If performance becomes an issue, I think this is a very straightforward option to parallelize using numba, but this might not become relevant unless you want to add better support for larger tables with more balls or something. Overall, I think that having a loading step before opening the GUI is probably the way to go, and anyone running large simulations from a program will just have the first run take slightly longer.

Also, I think I configured something weird on my terminal, so I wouldn't worry too much about it not working for me. Since everything seems alright, I'm going to start going down my checklist 👍🏽

Answer 25 · 2024-07-27T18:36:25.000Z

@ekiefl just as a heads up, I haven't quite finished up my review, but I've been updating the comments section at the bottom, and that has most of the feedback that will be in the final review. Feel free to get started on those items while I'm finishing up the final checks, and that should help reduce the turnaround time for acceptance 🚀

Edit: things got a little crazy on my end, but should be able to finalize the review before the weekend 👍

Answer 26 · 2024-07-28T21:37:43.000Z

Thanks for the update @eliotwrobson and for your valuable comments! I'm already working on the revisions :)

Answer 27 · 2024-08-06T20:26:53.000Z

@ekiefl As a heads up, I found some time to finish my review! It looks like you were able to address most of the items from my previous round of comments. The only things remaining from my end are the following:

I can see the code coverage badge but it doesn't show the coverage itself, maybe you just need to push again or something? I don't think this is really a huge deal.
I think that the JOSS submission is solid, but having some information about the usage of the package would be nice. Including some code from a vignette would be a good way to add this without too much extra effort (which ties into the third item), and maybe having some screenshots from the GUI would be cool too.
The other remaining checklist items are all related to vignettes for the package. I was able to get the game interface working reasonably well, and the discussion in the documentation was easy to follow, but I couldn't find vignettes for a simulation task that might arise in research not using the GUI. Having a couple of these would be awesome.

Answer 28 · 2024-08-07T05:15:42.000Z

Response to reviewers (take one)

Thanks again for your reviews @sebastianotronto and @eliotwrobson. I'll respond to your reviews in the same post.

And just a warning, @eliotwrobson, I had written the majority of this before you had finished your review, so I discuss unchecked boxes that are now checked. So some of this you can skip straight over, but I thought I would keep it just for posterity.

Common themes

Reading through everything, there are a couple of major themes that I'm picking up on that I want to address at a high level. Those things are versioning, ease of installation, and lack of obvious research application.

(1) Versioning

Thanks both of you for pointing out that pooltool doesn't work on Python 3.12. The culprit was the dependency pprofile, which is used in a couple of places in sandbox/. I think it's a great package, but until vpelletier/pprofile#51 is merged, I'm forced to drop it as a dependency. Any profiling code has been removed, rather than replaced with something else ¯_(ツ)_/¯.

Package dependencies are now defined by poetry. I did some manual dependency version testing and settled on the following dependencies:

[tool.poetry.dependencies]
python = ">=3.9,<3.13"
panda3d = [
  {platform = "darwin", version=">=1.10.13,<1.11"},
  {platform = "linux", version = "1.11.0.dev3444", allow-prereleases = true, source = "panda3d-archive"},
  {platform = "win32", version = "1.11.0.dev3444", allow-prereleases = true, source = "panda3d-archive"},
]
panda3d-gltf = ">=1.2.0"
panda3d-simplepbr = ">=0.12.0"
numpy = ">=1.26.0"  # Lower bound for 3.12 (https://github.com/numpy/numpy/releases/tag/v1.26.0)
numba = ">=0.59.0"  # # Lower bound for 3.12 (https://numba.readthedocs.io/en/latest/user/installing.html#version-support-information)
scipy = ">=1.12.0"  # Required for numba. Lower bound for 3.12 is officially 1.11, but in practice seems to be 1.12 on MacOS
attrs = ">=21.3.0"
cattrs = ">=22.1.0"
msgpack = ">=1.0.0"  # cattrs structuring fails with msgpack<1
msgpack-numpy = ">=0.4.8"
pyyaml = ">=5.2"
click = ">=8.0.0"
Pillow = ">=6.2.0"
h5py = ">=3.10"

The lower bounds were determined judiciously, where I tried to pick the oldest versions that were compatible with Python 3.12. This maximizes the target space for dependency resolution so as to maximize the odds that pooltool can share the same python environment with other packages, rather than being relegated as a standalone application. I could have taken this even further by creating Python version conditioned dependencies (e.g. if Python <3.9, use Numpy <1.26), but I thought that was too much to manage for such a small project.

Speaking of which, numpy 1.26.0 is the lower bound for 3.12, but it drops support for 3.8. I've followed suit and dropped support for 3.8, which makes pooltool's Python range >=3.9,<3.13.

To test over this version range, I am now running pooltool's test suite in a matrix of builds, where I test 3.9 and 3.12 for Linux, MacOS, and Windows. With this test matrix now in place, testing for 3.13 and beyond will be easy-peasy.

The development version has correspondingly experienced a massive bump from 3.8.10 to 3.12.4. No more living in the past!

All the changes mentioned can be found in ekiefl/pooltool#124 and ekiefl/pooltool#125.

(2) Ease of installation

Installation got mentioned quite a few times, which obviously isn't a great sign. For me, the main takeaways from reading your comments were that installation (1) doesn't work on 3.12, (2) perhaps messy/long, and (3) somewhat hard to find.

As mentioned, pooltool now works on 3.12. There also now exists improved signage in the installation instructions and in the README.md that clarify which Python versions pooltool supports.

The pip installation instructions previously had an unconventional additional step (for Linux and Windows), requiring uninstallation of panda3d 1.10.x and reinstallation of 1.11.x--a prerelease. For context, this is required due to this mouse-related feature being absent in Windows and Linux: panda3d/panda3d#928. Thanks to poetry, this messiness is now handled with the following dependency specification:

panda3d = [
  {platform = "darwin", version=">=1.10.13,<1.11"},
  {platform = "linux", version = "1.11.0.dev3444", allow-prereleases = true, source = "panda3d-archive"},
  {platform = "win32", version = "1.11.0.dev3444", allow-prereleases = true, source = "panda3d-archive"},
]

The result is now that what used to be this:

# Windows and Linux
pip install pooltool-billiards
pip uninstall panda3d -y
pip install --pre --extra-index-url https://archive.panda3d.org/ panda3d

# MacOS
pip install pooltool-billiards

Is now this:

# Windows and Linux
pip install pooltool-billiards --extra-index-url https://archive.panda3d.org/

# MacOS
pip install pooltool-billiards

Once 1.11 is released, the --extra-index-url can be removed altogether :)

The developer instructions have also been improved and shortened thanks to this and thanks to poetry.

(3) Lack of obvious research application

It seems that pooltool has not been presented in a way that exemplifies its use in research. First of all, it would be very helpful to hear what both of you have to say about this, since this wasn't expanded upon in either of your comments, I'm currently just inferring this based on the unchecked box related to whether pooltool satisfies JOSS's definition of having an obvious research application.

So further elaboration would be very helpful. But in the meantime, I would like to bring to light several things that in my opinion justify not only pooltool's potential to be applied in research, but also its already demonstrated application within research.

If you haven't already read it, I tried my best in the JOSS draft paper to describe billiards-related research and how billiards simulation plays a central role in this multidisciplinary field. (By the way, there exists a GitHub action that renders this paper, so if you want to view it rendered, you can download the artifact from this run). Anyways, here are some relevant excerpts that help define the research landscape of billiards:

Excerpt 1:

Billiards, a broad classification for games like pool and snooker, supports a robust, multidisciplinary research and engineering community that investigates topics in physics, game theory, computer vision, robotics, and cue sports analytics. Central to these pursuits is the need for accurate simulation.

Excerpt 2:

Billiards simulation serves as the foundation for a wide array of research topics that collectively encompass billiards-related studies. Specifically, the application of game theory to develop AI billiards players has led to simulations becoming critical environments for the training of autonomous agents [@Smith2007-jq; @Archibald2010-av; @Fragkiadaki2015-oh; @Archibald2016-sd; @Silva2018-cm; @Chen2019-dk; @Tung2019-zu]. Meanwhile, billiards-playing robot research, which relies on simulations to predict the outcome of potential actions, has progressed significantly in the last 30 years and serves as a benchmark for broader advancements within sports robotics [@Sang1994-jv; @Alian2004-zs; @Greenspan2008-wg; @Nierhoff2012-st; @Mathavan2016-ck; @Bhagat2018-bx]. Billiards simulations also enrich computer vision (CV) capabilities, facilitating precise ball trajectory tracking and enhancing shot reconstruction for player analysis and training (for a review, see @Rodriguez-Lozano2023-hq). Additionally, through augmented reality (AR) and broadcast overlays, simulations have the potential to extend their impact by offering shot prediction and strategy formulation in contexts such as personal training apps and TV broadcasting, creating a more immersive understanding of the game.

Since billiards-related research is niche, I can understand how pooltool's application to research may not be obvious, however I think the "Statement of Need" section explains things by explaining the current shortcomings of the research landscape:

Unfortunately, the current billiards simulation software landscape reveals a stark contrast between the realistic physics seen in some commercially-produced games (i.e., Shooterspool and VirtualPool4) and the limited functionality of open-source projects. Commercial products have little, if any, utility in research contexts due to closed source code and a lack of open APIs. Conversely, available open source tools lack realism, usability, and adaptability for generic research needs. The most widely cited simulator in research studies, FastFiz[^1], is unpackaged, unmaintained, provides no modularity for custom geometries nor for physical models, offers restrictive 2D visualizations, outputs minimal simulation results with no built-in capabilities for introspection, and was custom built for hosting the Association for the Advancement of Artificial Intelligence (AAAI) Computational Pool Tournament from 2005-2008 [@Archibald2010-av]. Another option, Billiards[^2], offers a visually appealing 3D game experience, realistic physics, and supports customization via Lua scripting. However, as a standalone application, it lacks interoperability with commonly used systems and tools in research. Written in Lua, an uncommon language in the scientific community, it has limited appeal in research settings. The lack of Windows support is another drawback. FooBilliard++[^3] is a 3D game with realistic physics, yet is not a general purpose billiards simulator, instead focusing on game experience and aesthetics. Other offerings suffer from drawbacks already mentioned.

The lack of suitable software for billiards simulation in research contexts forces researchers to develop case-specific simulators that meet their research requirements but fall short of serving the broader community as general purpose simulators. This fragments the research collective, renders cross-study results difficult or impossible to compare, and leads to wasted effort spent reinventing the wheel. pooltool fills this niche by providing a billiards simulation platform designed for speed, flexibility, and extensibility in mind.

Pooltool was created specifically to fill in this gap in research tools.

But perhaps more importantly, I would like to point out the ways in which pooltool has already been applied in research:

Researchers at the Shanghai AI Laboratory have developed pool-playing AI agents using pooltool as the simulation environment. Pooltool has been incorporated as a formal dependency in their popular MCTS-based RL project, LightZero. In the words of one of their main developers:

The pooltool simulation environment [...] holds great potential as a research benchmark in the field of reinforcement learning. It not only offers scientific value but also encompasses an element of fun. We believe integrating it as a long-term expandable benchmark environment will significantly contribute to the advancement of our research endeavors

(opendilab/LightZero#182 (comment))
Researchers at the University of Edinburgh are using pooltool to test the physical reasoning capabilities of large language models (LLMs). The precedent for this research is described by them in SimLM, and a small teaser of their work using pooltool is described here.
A student at Oxford University, Alistair White-Horne, created a "Real-Time Pool and Snooker Assistant" using pooltool as the underlying visualization and simulation module. Their thesis can be found here

One thing that could highlight pooltool as a research tool could be turning some analyses in sandbox/ into proper vignettes that are hosted on the documentation. For example, the contributor @zhaodong-wang added a quantiative verification of the "30-degree rule", a practical rule for determining ball directions post-collision: ekiefl/pooltool#119. This could be a two birds one stone effort, since @eliotwrobson rightfully noticed a lack of vignettes. How does that sound?

Also, perhaps a section could also be added to the README.md called "Projects using pooltool" or something like that, since referencing ways in which pooltool is being used in research environments is both proof of and advertisement for pooltool's applicability to research.

The checklist

With those three large topics out of the way, here is the checklist section of my response.

Since the checklist is the same for both of you, I'm going to respond to each unchecked box and tag one or both of you depending on whether you had checked the box. In the next sections I'll address each of your individual comments.

Vignette(s) demonstrating major functionality that runs successfully locally.

@eliotwrobson, I haven't done it yet, but I'm going to add a couple of vignettes to the documentation, since currently there is really just the "Hello World" vignette.

Examples for all user-facing functions.

How should we proceed @cmarmo? @sebastianotronto is right that not every user-facing function contains an example. But as they've pointed out, that's because the API is very granular: "As I noted above, not all functions have examples, but considering how fine-grained the API is I do not think it would be necessary".

The vast majority of the codebase is exposed in the API, so if we take the term "user-facing functions" to mean those in the API, this requirement would entail an enormous amount of work. In my opinion, the effort would extend beyond what I assume to be the intent of this checkbox: to ensure users have resources to learn by example. For what it's worth, I think the documentation in its current state satisfies this very important requirement (example-based documentation is something I care a lot about).

Metadata including author(s), author e-mail(s), a url, and any other relevant metadata e.g., in a pyproject.toml file or elsewhere.

@sebastianotronto, @eliotwrobson: Added in ekiefl/pooltool#124. Can be seen here: https://github.com/ekiefl/pooltool/blob/3b76a311166dba32bd34771fe2ab82b6e3fbf71e/pyproject.toml#L19-L37

Badges for:

Continuous integration and test coverage,

Docs building (if you have a documentation website),

A repostatus.org badge,

Python versions supported,

Current package version (on PyPI / Conda).

@sebastianotronto, @eliotwrobson: Added in ekiefl/pooltool#127. Can be seen here: https://github.com/ekiefl/pooltool

Package installation instructions

@sebastianotronto, @eliotwrobson: I read you guys loud and clear, the installation instructions definitely need more visibility. But I would prefer that they are not duplicated in both the README.md and the documentation (https://pooltool.readthedocs.io/en/latest/getting_started/install.html). This increases maintenace burden by violating single-source-of-truth. To achieve what I hope is the best of both worlds, I've added an explicit # Installation section to the README that links to the installation page in ekiefl/pooltool#127. In particular, this commit: ekiefl/pooltool@79085c3

Any additional setup required to use the package (authentication tokens, etc.)

@sebastianotronto, @eliotwrobson: With the adoption of poetry, I hope there now exists no additional setup that needs documenting (I say that with my fingers crossed).

Descriptive links to all vignettes. If the package is small, there may only be a need for one vignette which could be placed in the README.md file.

Brief demonstration of package usage (as it makes sense - links to vignettes could also suffice here if package description is clear)

@sebastianotronto, @eliotwrobson: Once vignettes are added to the documentation, I will link them somewhere in the README.md

If applicable, how the package compares to other similar packages and/or how it relates to other packages in the scientific ecosystem.

@sebastianotronto, here is a paragraph from the JOSS paper.md draft that situates pooltool with respective to similar software:

"Unfortunately, the current billiards simulation software landscape reveals a stark contrast between the realistic physics seen in some commercially-produced games (i.e., Shooterspool and VirtualPool4) and the limited functionality of open-source projects. Commercial products have little, if any, utility in research contexts due to closed source code and a lack of open APIs. Conversely, available open source tools lack realism, usability, and adaptability for generic research needs. The most widely cited simulator in research studies, FastFiz[^1], is unpackaged, unmaintained, provides no modularity for custom geometries nor for physical models, offers restrictive 2D visualizations, outputs minimal simulation results with no built-in capabilities for introspection, and was custom built for hosting the Association for the Advancement of Artificial Intelligence (AAAI) Computational Pool Tournament from 2005-2008 [@Archibald2010-av]. Another option, Billiards[^2], offers a visually appealing 3D game experience, realistic physics, and supports customization via Lua scripting. However, as a standalone application, it lacks interoperability with commonly used systems and tools in research. Written in Lua, an uncommon language in the scientific community, it has limited appeal in research settings. The lack of Windows support is another drawback. FooBilliard++[^3] is a 3D game with realistic physics, yet is not a general purpose billiards simulator, instead focusing on game experience and aesthetics. Other offerings suffer from drawbacks already mentioned."

Do you think I should try and synthesize some of this info into the README, or do you think it's fine to let sleeping dogs rest?

Citation information

@sebastianotronto: Added in ekiefl/pooltool#127 (ekiefl/pooltool@11e9efd).

The package is easy to install

@eliotwrobson, I think the instructions (both from source and with pip) are simpler and easier to find now!

Packaging guidelines: The package conforms to the pyOpenSci packaging guidelines.
A few notable highlights to look at:

Package supports modern versions of Python and not End of life versions.

Code format is standard throughout package and follows PEP 8 guidelines (CI tests for linting pass)

@sebastianotronto, @eliotwrobson: I think things are now much more compliant. PRs ekiefl/pooltool#124, ekiefl/pooltool#125, ekiefl/pooltool#127, ekiefl/pooltool#128, and ekiefl/pooltool#129 have transformed pooltool into a package that, at least from what I can tell, complies with the package guidelines set forth by pyOpenSci. There are (at least) two exceptions though: pooltool has no developer guide (described as ideal to have) and no conda package (which I am keen to add, but currently is blocked by this grayskull issue: conda/grayskull#463).

The package has an obvious research application according to JOSS's definition in their submission requirements.

Note: Be sure to check this carefully, as JOSS's submission requirements and scope differ from pyOpenSci's in terms of what types of packages are accepted.

The package contains a paper.md matching JOSS's requirements with:

A short summary describing the high-level functionality of the software

Authors: A list of authors with their affiliations

A statement of need clearly stating problems the software is designed to solve and its target audience.

References: With DOIs for all those that have one (e.g. papers, datasets, software).

@sebastianotronto, @eliotwrobson: I gave my spiel about this above in "Common themes".

Comments by @sebastianotronto

The tool also offers a graphical mode that can be "played" like a 3D game. The overall effect is very realistic and it appears clear that the physics is accurately simulated. I also liked the mouse + keyboard controls, personally I found them intuitive.

Thanks for your kind words :)

The author does not make specific claims about performance, but the graphical mode was quit demanding on my laptop, with extreme lag. On the other hand, it worked like a charm on my much more powerful desktop.

Yeah, I've noticed some hardware can't quite manage the GUI.

Help menu (but also other text) is cut out when playing with unusual resolution (I was keeping two windows side by side)

The GUI assumes a specific aspect ratio. For example, if users change the window size by clicking and dragging corners, an area-preserving transformation takes place to ensure the ratio. But if there are additional constraints that force a different aspect ratio, we're out of luck.

Stick disappeared after entering visual mode, only reappears in shot mode or English mode.

I appreciate the feedback on playability. It's nice to know what people find intuitive/unintuitive. In this case, I modelled the behavior after VirtualPool4, so this isn't actually a bug.

I have also been thinking about some possible performance improvement in the main simulation loop. I don't know if my suggestion would actually lead to any significant improvement, but it is interesting to think about it.

I mentioned this in my last post, but this is a really good idea! I will try and implement this and get back to you with the results.

Comments by @eliotwrobson

For a package this size, it would be nice to have all of the tests in a separate folder.

I've never worked with a separate tests folder, but it's about time I made the change. I will implement this.

The API documentation seems very complete, but the formatting is a little awkward and hard to read. For example, this indentation looks strange:

Agreed. I feel like the closing bracket should be left indented one more than it is. IIRC I was having trouble getting it properly formatted. If I can't get it to work I should just drop the multi-line call signature format and just do single line with line breaks, like pandas: https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.read_csv.html.

Since there are many large functions, it might be easier to switch to giving each function it's own page, the way that the numpy docs are set up.

I hear you, and I agree that would be nice. But the furo sphinx theme used by pooltool is well known and trusted for big projects like pip, attrs, urllib3, black, and more. So this may be a matter of taste. I would also add that I am open to to experimenting with different formats, but my journey with doc rendering not been a smooth, so I am resistant to doing it myself.

I think there needs to be a couple more vignettes. It would be awesome to have a few that show tasks that could arise in some type of research.

Good call. Thanks for pointing this out. I feel like these vignettes will really help highlight the research elements of pooltool, which is ultimately what it was designed for.

I know that the type annotations aren't read by numba and that library has it's own type conversions, but it would be nice to have them on the numba jit-ed functions to make them easier to read.

This would be helpful for me, too! I will also experiment with specifying the types according to their string-based system: https://numba.pydata.org/numba-doc/latest/reference/types.html

I can see the code coverage badge but it doesn't show the coverage itself, maybe you just need to push again or something? I don't think this is really a huge deal.

Yeah, this was annoying! I will poke around more.

I think that the JOSS submission is solid, but having some information about the usage of the package would be nice. Including some code from a vignette would be a good way to add this without too much extra effort (which ties into the third item), and maybe having some screenshots from the GUI would be cool too.

Good call. I'll revisit the text after I finish the other bullet points.

The other remaining checklist items are all related to vignettes for the package. I was able to get the game interface working reasonably well, and the discussion in the documentation was easy to follow, but I couldn't find vignettes for a simulation task that might arise in research not using the GUI. Having a couple of these would be awesome.

Yeah, I'm totally on board with that.

My TODOs

I wanted to get my response out before I had necessarily addressed all the issues, just to keep things out in the open. I think getting it out early could prove especially useful for getting feedback on my plan, just to make sure I'm not barking up the wrong tree. So I've made this to-do list, which I welcome anyone to make suggestions to.

Implement a compiling/loading numba cache step so initial shot doesn't take forever
Improve the formatting of documentation
Move test modules to tests/
Deep dive on @sebastianotronto's idea for event caching
Add numba types
Add research-oriented vignettes to docs
code coverage badge bug?
Consider adding to the paper.md

Answer 29 · 2024-08-07T07:45:53.000Z

Thank you @ekiefl for your prompt answer and the todo summary!
I'm letting @sebastianotronto and @eliotwrobson comment about your todo list.

is right that not every user-facing function contains an example. But as they've pointed out, that's because the API is very granular: "As I noted above, not all functions have examples, but considering how fine-grained the API is I do not think it would be necessary".

We experienced similar situations with other consistent packages. As we try to enforce a review schedule, my suggestion here is to add some of the missing examples to the documentation, in order to signify the "work in progress" status, and let the rest for future improvements.... you may even want to organize some sprints and call for contributors in that case...

I have labeled this issue as "awaiting changes" : let's touch base in three weeks, at August 28th, to see if reviewers are happy with your changes.

Does this sound reasonable?

Answer 30 · 2024-08-07T10:20:16.000Z

Hi @ekiefl, thank you for your response!

After your comment I checked the recent changes and ticked a few more checkboxes. Also, I would like to point out that some of the unchecked boxes are (or were) so because the requested information was not present in the README file. But I agree with you that having it in the docs and linking to it in the README is a good approach. @cmarmo can the requirements be considered satisfied if the information is only linked to in the README?

Thanks for improving the installation process, I tested it now and the one-liner works!

As for the research applications, I admit that at the time of my first review I did not understand that the project was also submitted to JOSS, hence the missing ticks. The goal of the project is clear to me and I understand the research applications, all good on that side!

For now I am very happy with the changes you made, I'll check back in a couple of weeks for the items in your to-do list :)

Answer 31 · 2024-08-12T04:35:11.000Z

Still reading but all of your proposed solutions for issue 3 sound really good 👍🏽 Mentioning something about these in your JOSS paper would make it stronger as well.

Answer 32 · 2024-08-12T07:56:26.000Z

can the requirements be considered satisfied if the information is only linked to in the README?

@sebastianotronto sure! The important thing is that the information is accessible from the README: links are an acceptable option.

Code of Conduct & Commitment to Maintain Package

Description

Scope

Domain Specific

Community Partnerships

Technical checks

Publication Options

Are you OK with Reviewers Submitting Issues and/or pull requests to your Repo Directly?

Please fill out our survey

Editor and Review Templates

Footnotes

Editor in Chief checks

Editor comments

Package Review

Documentation

Usability

Functionality

For packages also submitting to JOSS

Final approval (post-review)

Review Comments

Installation

Graphical mode

Documentation and API

Code

Possible performance improvement

Package Review

Documentation

Usability

Functionality

For packages also submitting to JOSS

Final approval (post-review)

Review Comments

Response to reviewers (take one)

Common themes

(1) Versioning

(2) Ease of installation

(3) Lack of obvious research application

The checklist

Comments by @sebastianotronto

Comments by @eliotwrobson

My TODOs