search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
721 stars 38 forks source link

[REVIEW]: CellPyLib: A Python Library for working with Cellular Automata #3608

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @lantunes (Luis Antunes) Repository: https://github.com/lantunes/cellpylib Version: v2.0.0 Editor: @jbytecode Reviewers: @blsqr, @szhorvat Archive: 10.6084/m9.figshare.16977619

: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

Status badge code:

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

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

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

  1. Make sure you're logged in to your GitHub account
  2. 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 @jbytecode 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 @blsqr

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @szhorvat

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 3 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @blsqr, @szhorvat 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:

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

watching

  1. 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

Wordcount for paper.md is 892

whedon commented 3 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.07 s (839.2 files/s, 72644.5 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          37            518            479           2603
Markdown                         2            122              0            299
reStructuredText                17            283            436            282
TeX                              1              7              0             87
DOS Batch                        1              8              1             26
make                             1              5              7             16
YAML                             1              2              0             13
-------------------------------------------------------------------------------
SUM:                            60            945            923           3326
-------------------------------------------------------------------------------

Statistical information for the repository 'b3213f420f3556f11c9f074e' was
gathered on 2021/08/12.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Luis Antunes                    53         18235          14706           99.75
Martin Neumayer                  3            76              5            0.25

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Luis Antunes               3528           19.3         25.4                5.10
Martin Neumayer              72           94.7          4.7                0.00
whedon commented 3 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1142/4702 is OK
- 10.1016/0167-2789(90)90064-v is OK
- 10.1038/s41586-020-2649-2 is OK
- 10.1109/MCSE.2007.55 is OK
- 10.25088/complexsystems.26.4.319 is OK

MISSING DOIs

- None

INVALID DOIs

- None
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:

jbytecode commented 3 years ago

Dear reviewers @blsqr and @szhorvat

Thank you for accepting our invitation.

This is the reviewing page for the current submission. In this page, there are 20 checkboxes for each reviewer. These boxes indicate the corresponding reviewing tasks. Please fill them whenever you complete the corresponding task.

Please have a look at the Reviewing Guidelines page for details of reviewing process of JOSS.

Please do not hesitate to ask me anything.

jbytecode commented 3 years ago

@whedon add @martibosch as reviewer

whedon commented 3 years ago

OK, @martibosch is now a reviewer

jbytecode commented 3 years ago

@martibosch I also assigned you as the third reviewer. Thank you for accepting our invitation.

jbytecode commented 3 years ago

@whedon generate pdf

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: @szhorvat, please update us on how your review is going (this is an automated reminder).

whedon commented 3 years ago

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

jbytecode commented 3 years ago

@szhorvat 👋, @blsqr 👋, whedon automatically reminds the reviewers, but since you have informed us about your availability, you can skip the reminder.

@martibosch since you didn't set a deadline, could you please inform us how is going your review?

blsqr commented 3 years ago

I just opened an issue regarding minor corrections to references and citation style.

jbytecode commented 3 years ago

Dear @blsqr 👋, @szhorvat 👋, and @martibosch 👋

I'm just pinging you to ask how your review is going. Will you have the opportunity to start this week?

Thank you in advance and best wishes!

szhorvat commented 3 years ago

Yes, I can finish by the end of next week (17th), as originally planned.

blsqr commented 3 years ago

I will continue with reviewing on the 20th.

martibosch commented 3 years ago

Hello @jbytecode,

I will try to complete the review by the end of this week. Best,

Martí

jbytecode commented 3 years ago

@lantunes would you consider adding a GitHub action that builds and tests the package after each commit so people can see the latest changes don't break the stuff?

szhorvat commented 3 years ago

I have not been able to finish by Friday as planned, but I am looking at it now, and should be able to complete it soon.

szhorvat commented 3 years ago

I need a bit of help from @lantunes or reviewers who are more experienced with Python. Is it expected that animations (such as plot2d_animate from CellPyLib) won't work out of the box in Jupyter notebooks, when using inline graphics?

szhorvat commented 3 years ago

I opened a few issues in the CellPyLib repository, and I'd like to wait for responses from @lantunes before proceeding.

So far I enjoy playing with the library. There are nice tutorials which demonstrate classical concepts. The library is definitely useful to learning and teaching. Now I am trying to determine how suitable it is for research: exploring novel ideas that may not have been tried yet.

A general comment/suggestion to @lantunes (and to be clear: this is certainly not something I ask to be fixed before publication in JOSS) is to think about which directions you want to go in in the future, and whether the current design of the library will generalize to incorporate new features. A few interesting things that might be added in the future:

If you consider all the different possibilities now, and decide which you might want to support in the future, it will help you come up with a future-proof design.

szhorvat commented 3 years ago
  • 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

I cannot check off this point without improvements. Specifically:

  1. There are basic steps for building the docs and running tests, but nothing explicit about whether contributions are welcome or how to contribute. This needs to be improved.
  2. This is implicit in the sense that the project is on GitHub, so people will assume they can use the issue tracker. IMO this is fine.
  3. There is no information about where to seek support. This forced me to start asking questions on the issue tracker, which most other projects would not allow. This must be improved.
szhorvat commented 3 years ago
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

https://cellpylib.org/ has a very clear and concise description of what the software can do right now. Great job on this! However, it would be nice to indicate what the goals (as opposed to current capabilities) of the software project is, and who the target audience is. Is it trying to be a learning/teaching tool? I notice that it has built-in implementations of some classic models. Is it trying to be a research tool? Does it focus more on flexibility/generality or on performance (usually, there is a tradeoff between the two)? Does this project have the ambition to become the primary CA library for Python, which means that there should be a push to complete missing features?

While the current description is good, I feel that it can be improved by describing the future vision for the project. This will aid potential users who need to decide whether to invest into this library for the long term.

jbytecode commented 3 years ago

@lantunes could you please ping us (and if possible please update your status) if you receive the notifications from this issue?

lantunes commented 3 years ago

Thank you for all of your comments and review thus far! I have been following the discussion, though haven't yet begun the process of addressing the individual items. I will begin addressing the items today, and aim to be done by the weekend.

lantunes commented 3 years ago

@lantunes would you consider adding a GitHub action that builds and tests the package after each commit so people can see the latest changes don't break the stuff?

@jbytecode Certainly! I will look into adding this.

lantunes commented 3 years ago

I have added a GitHub action to the cellpylib repo that runs the tests on any push to main. I have also added a status badge to the top of the project's README. @jbytecode

blsqr commented 3 years ago

@whedon generate pdf

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:

blsqr commented 3 years ago

I opened a few additional issues in the cellpylib project and would also pause my review at this point, continuing next week.

I believe I have arrived at a rather similar point as @szhorvat above, where I am unclear about the scope of and long-term vision for the project. From what I've seen so far, cellpylib is well-suited for exploring CA-based complex systems and conveying principles of established examples from the scientific literature. Particularly the large number of tutorials and the well-written documentation are very inviting and enjoyable to read and follow along in code.

However, depending on the type of research that cellpylib aims to form the operational basis of, a number of additional features seem to become relevant; I would concur with the list given by @szhorvat, especially with function generalisations, different lattice types, and recursive rules. Furthermore, performance aspects may become increasingly important. This clearly can reach an unfeasibly large scope very easily…

@lantunes Can you perhaps comment on these points (here or in a separate issue) such that we can better align the review of the package to its overarching goals?

jbytecode commented 3 years ago

Dear @martibosch, could you please update your status on how is your reviewing going?

martibosch commented 3 years ago

Hello @jbytecode,

unluckily I have been swamped these last weeks. Since there are already two reviewers, I suggest that you move forward with their comments since I will not have the time to properly review this library this week. I am sorry for any inconveniences.

Best, Martí

lantunes commented 3 years ago

@szhorvat @blsqr Thank you for all your thoughts thus far. Regarding your comments on the future directions of the project, you certainly bring up very relevant and valid issues. The field of CA is vast, and there are indeed a great number of variations on the basic concept introduced decades ago, from stochastic behaviour to arbitrary underlying network structure, as you describe. I've thought about this in the past: how can one design a general-purpose CA library that reflects the full richness of the field? What I concluded is that there is a trade-off to be made in terms of straightforward and intuitive design and flexibility of model construction. In other words, if we want a library that can support the various kinds of CA that have been investigated in the literature, then the design of the library would necessarily be more complex, and possibly less intuitive to use. (There are likely other trade-offs as well, such as between flexibility and performance: as the library becomes more flexible in terms of the kinds of models one can construct, then it becomes more difficult to provide a performant implementation for any use case.)

To investigate this idea, I actually began creating another library some time ago, called Netomaton (https://github.com/lantunes/netomaton). Netomaton supports many more use cases than CellPyLib, including some of those in your list, @szhorvat. While some may find this library intuitive, I find that one needs to operate in the world of networks (this was the abstraction I chose; arguably, one can choose a better paradigm), and this may impose a burden on the user who is simply wishing to work with elementary CA. Moreover, I found that there are fewer opportunities to improve the performance with the underlying network abstraction.

In summary, I would say that the future scope of the project will not deviate too far from the kinds of systems that are currently supported. I am certainly open to supporting more model types and use cases, but not to the point that it requires a large redesign of the API. Elementary CA, and perhaps the other kinds of CA in the examples, are still very much a topic of research today, so even with some missing features, the library still has applicability in a research setting, in my opinion.

blsqr commented 3 years ago

Thank you for the thorough response, @lantunes, and the pointer to Netomaton (which looks interesting as well!). I think the differences in abstractions between grid-based and network-based automata is a natural point at which to distinguish the libraries, with CellPyLib focussing on grid-based automata. As said, the trade-offs become substantial if aiming to do both…

I think the term "general-purpose" (also used in the software paper) is what may have let me astray, given the many different approaches to CA-based research and the corresponding trade-offs. I would propose to extend the description of CellPyLib and the statement of need such that it is clearer that CellPyLib puts a focus on a subset of that field (elementary CA, CA with totalistic rules, …) and aims for a user-friendly interface and optimised and tested algorithms. In my eyes – given the vastness of the field – this would clearly be a feature of the library rather than a shortcoming, so I would highlight it accordingly. I think it's important for potential users to know what they can and cannot do with the library before spending time on getting to know and using it.

lantunes commented 3 years ago

@jbytecode @szhorvat @blsqr Thank you for all the suggestions thus far. They have been very constructive, and improve the library greatly. I am continuing to address the feedback in the cellpylib GitHub repo. I assumed I would have been done sooner, but other commitments have required more of my time. I hope to complete addressing all the feedback over the coming week.

szhorvat commented 3 years ago

@lantunes I declined the "invitation to collaborate" you sent through GitHub, as I assumed it was a mistake (we don't need write access to the repository in order to do the review). Let me know if I misunderstood something. Just wanted to clarify :-)

lantunes commented 3 years ago

@lantunes I declined the "invitation to collaborate" you sent through GitHub, as I assumed it was a mistake (we don't need write access to the repository in order to do the review). Let me know if I misunderstood something. Just wanted to clarify :-)

@szhorvat No worries! I added you as a collaborator so that I can add you as a reviewer on the pull request I opened in the repo. I just wanted to notify you about the pull request, which you can find here: https://github.com/lantunes/cellpylib/pull/23.

lantunes commented 3 years ago

@blsqr regarding your comment about the scope of the library, I have added a sentence to the README.md, and a Scope section to the paper, to clarify this issue. The relevant commit is here: https://github.com/lantunes/cellpylib/pull/23/commits/336207968dfe8d9cd46120d10f9cd6184e4b45e3.

lantunes commented 3 years ago

@jbytecode @szhorvat @blsqr I believe I have addressed all the feedback I have received thus far. Thank you again for your comments and suggestions, as they have been extremely valuable. I also apologize again for taking as long as I have to address the feedback.

The feedback has been addressed either in this thread or in the JOSS issues opened in the CellPyLib GitHub project. All the resulting code changes are in this PR: https://github.com/lantunes/cellpylib/pull/23.

jbytecode commented 3 years ago

@whedon generate pdf from branch joss_review

whedon commented 3 years ago 
Attempting PDF compilation from custom branch joss_review. 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:

blsqr commented 3 years ago

Thanks, @lantunes! I have left a few minor comments in https://github.com/lantunes/cellpylib/pull/23#pullrequestreview-782006416 and have opened an issue (https://github.com/lantunes/cellpylib/issues/24) regarding my last open point in the review checklist.

jbytecode commented 3 years ago

@blsqr thank you for your progressive reviewing & contributions and your great effort.

@szhorvat, @martibosch - 👋👋👋 - we are a little bit ahead of normal reviewing timings. could you please continue and finalize your reviewing? thank you in advance.

szhorvat commented 3 years ago

I'm travelling at the moment. I hope to finish it next week. Everything looks good 👍 I'd just like a implement one more CA to see if it's possible before finalizing everything.

szhorvat commented 3 years ago

@lantunes @blsqr Does the test suite pass for you when using the current joss_review branch?

I get:

AttributeError: 'PathCollection' object has no property 'depthshade'

It is coming from:

https://github.com/lantunes/cellpylib/blob/joss_review/cellpylib/ca_functions2d.py#L158

lantunes commented 3 years ago

@szhorvat The test suite is passing for me locally on the joss_review branch. The test suite is also passing when run during the Github action workflow for the project. What version of Matplotlib are you using in your environment?

szhorvat commented 3 years ago

I'm sorry, I thought I mentioned that already. It's version 3.4.2.