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]: Empirical: A scientific software library for research, education, and public engagement #6617

Closed editorialbot closed 5 months ago

editorialbot commented 7 months ago

Submitting author: !--author-handle-->@mmore500<!--end-author-handle-- (Matthew Andres Moreno) Repository: https://github.com/devosoft/Empirical/ Branch with paper.md (empty if default branch): joss-paper Version: v1.1.5 Editor: !--editor-->@mahfuz05062<!--end-editor-- Reviewers: @LTLA, @bramvandijk88 Archive: 10.5281/zenodo.11420797

Status

status

Status badge code:

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

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

@LTLA & @bramvandijk88, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @mahfuz05062 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 ✨

Checklists

📝 Checklist for @bramvandijk88

📝 Checklist for @LTLA

editorialbot commented 7 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 7 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/3062341.3062363 is OK
- 10.1145/367701.367714 is OK
- 10.1145/2048147.2048224 is OK
- 10.1093/bib/bbp073 is OK
- 10.1186/1752-0509-6-96 is OK
- 10.1145/3205455.3205523 is OK
- 10.1162/artl_a_00284 is OK
- 10.1177/0037549712462620 is OK
- 10.1186/2194-3206-1-3 is OK
- 10.1145/3205651.3205780 is OK
- 10.5281/zenodo.4118608 is OK
- 10.1145/3185517 is OK
- 10.5281/zenodo.2575606 is OK
- 10.1038/nchem.1149 is OK
- 10.48550/ARXIV.2108.00382 is OK

MISSING DOIs

- No DOI given, and none found for title: Not so fast: Analyzing the performance of webassem...
- No DOI given, and none found for title: Squares: A Fast Counter-Based RNG
- No DOI given, and none found for title: LEARNING EVOLUTION AND THE NATURE OF SCIENCE USING...
- No DOI given, and none found for title: Netlogo: A simple environment for modeling complex...
- No DOI given, and none found for title: shiny: Web Application Framework for R
- No DOI given, and none found for title: shiny: Web Application Framework for R
- No DOI given, and none found for title: Suicide, signals, and symbionts: Evolving cooperat...
- No DOI given, and none found for title: Changing Environments Drive the Separation of Gene...
- No DOI given, and none found for title:  Open Science Philosophy

INVALID DOIs

- None
editorialbot commented 7 months ago

Software report:

github.com/AlDanial/cloc v 1.90  T=6.55 s (139.9 files/s, 26043.8 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                   307          14616          17491          59901
C++                            384          10217           8478          38101
JavaScript                      10             99             82           9981
Markdown                        47           1029              0           3212
make                            61            908            392           2346
HTML                            51            118             24            841
Bourne Shell                    42            178            204            592
TeX                              2             44              0            443
Dockerfile                       1             27             16            241
YAML                             2              4             18            233
Python                           4             61            108            176
LESS                             1             26             41            133
reStructuredText                 1             63              8            124
CSV                              2              0              0             56
JSON                             2              0              0             25
-------------------------------------------------------------------------------
SUM:                           917          27390          26862         116405
-------------------------------------------------------------------------------

Commit count by author:

  7566  Charles Ofria
  1186  Matthew Andres Moreno
   746  Emily Dolson
   247  Alexander Lalejini
   207  rodsan0
   172  Alex Lalejini
   113  Katherine Perry
    97  Austin Ferguson
    88  kayakingCellist
    85  Jake Fenton
    66  Tait Weicht
    55  mmore500.login+git@gmail.com
    45  Riley Hoffman
    41  Steven Jorgensen
    36  NateRiz
    31  emilydolson
    18  grenewode
    17  abbywlsn
     9  Robin Miller
     8  Oliver-BE
     7  anyaevostinar
     6  Jason Stredwick
     4  Replit user
     4  perryk12
     3  Jose
     3  Raheem Clemons
     2  Anya V
     2  Emily Louise Dolson
     2  Jory Schossau
     2  Luis Zaman
     2  Steven Patrick Jorgensen
     2  cgnitash
     2  mmore500
     1  Acacia Ackles
     1  Anya Johnson
     1  Anya Vostinar
     1  HackMD
     1  Santiago Rodriguez Papa
     1  c-moreno
     1  djrain
     1  leg2015
     1  ryan-moreno
editorialbot commented 7 months ago

Paper file info:

📄 Wordcount for paper.md is 1379

✅ The paper includes a Statement of need section

editorialbot commented 7 months ago

License info:

🟡 License found: Other (Check here for OSI approval)

editorialbot commented 7 months ago

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

mahfuz05062 commented 7 months ago

@LTLA and @bramvandijk88 - Thank you for agreeing to review this submission.

This is the review thread for the paper. All of our communications will happen here from now on.

As mentioned above, you can use the command @editorialbot generate my checklist to create your review checklist. As you go over the submission, please check any items that you feel have been satisfied.

There are also links to the JOSS reviewer guidelines (https://joss.readthedocs.io/en/latest/reviewer_guidelines.html)

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#6617 so that a link is created to this thread for visibility. Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if you require additional time. We can also use editorialbot (our bot) to set automatic reminders if you know you'll be away for a known period.

Please feel free to ping me (@mahfuz05062) if you have any questions/concerns.

bramvandijk88 commented 7 months ago

Review checklist for @bramvandijk88

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

bramvandijk88 commented 7 months ago

Heya, I've started my review in between some teaching obligations. I generally dislike debugging paths and compiling C code, so I'll leave the problem-solving up to the authors:

In following the installation instructions on Mac OSX (Ventura 13.2.1) I get the following missing file:

`Makefile:22: WARNING: the Cookiecutter Empirical Project, which you can find at https://github.com/devosoft/cookiecutter-empirical-project, should be preferred over ProjectTemplate g++ -O3 -DNDEBUG -Wall -Wno-unused-function -std=c++20 -I../../../Empirical/include/ source/native/project_name.cpp -o project_name In file included from source/native/project_name.cpp:21: In file included from ../../../Empirical/include/emp/config/command_line.hpp:42: In file included from ../../../Empirical/include/emp/config/../tools/string_utils.hpp:35: ../../../Empirical/include/emp/config/../tools/../base/array.hpp:28:10: fatal error: '../../../third-party/cereal/include/cereal/cereal.hpp' file not found

include "../../../third-party/cereal/include/cereal/cereal.hpp"

     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

1 error generated. make: *** [project_name] Error 1`

LTLA commented 6 months ago

Review checklist for @LTLA

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

LTLA commented 6 months ago

Alright, apologies for the delay. Here's my review:

This is a very interesting library. I have long wondered how to manipulate HTML from within C++, and Empirical provides an ergonomic way of doing so. It offers the possibility of writing web applications almost completely in C++, without any need to touch Javascript or associated frameworks. I can forsee this being useful for scientific programmers who want to build a simple application based on C++ tools without requiring any web development knowledge. In this respect, Empirical targets a similar market to Shiny (R) or dash (Python), but for C++ developers. That said, more experienced web developers will probably not use these features, as direct DOM manipulation is discouraged in frameworks like React.

The extra debugging information for the standard data structures is nice, especially for WebAssembly where debugging segfaults is even more tedious than usual. Perhaps the authors might consider checking whether a macro could be used to seamlessly switch between emp:: and std::. This would allow library developers to use Empirical for debug builds, but then switch back to the STL for release, in order to reduce dependencies for their own users. For example, if I was developing a header-only library that uses Empirical for debugging, I would prefer to avoid mandating Empirical as a compile-time dependency for all applications that use my library.

The paper itself is well-written and concise. Possibly too concise, actually: it might have benefited from one or two short code examples to demonstrate the ease-of-use of the UI manipulation, bounds-checking, etc. Then readers can get a better grasp of what the library actually does, before deciding whether to proceed to the documentation itself.

On some other minor points:

Related issues:

mmore500 commented 6 months ago

Heya, I've started my review in between some teaching obligations. I generally dislike debugging paths and compiling C code, so I'll leave the problem-solving up to the authors:

In following the installation instructions on Mac OSX (Ventura 13.2.1) I get the following missing file:

Makefile:22: WARNING: the Cookiecutter Empirical Project, which you can find at https://github.com/devosoft/cookiecutter-empirical-project, should be preferred over ProjectTemplate g++ -O3 -DNDEBUG -Wall -Wno-unused-function -std=c++20 -I../../../Empirical/include/ source/native/project_name.cpp -o project_name In file included from source/native/project_name.cpp:21: In file included from ../../../Empirical/include/emp/config/command_line.hpp:42: In file included from ../../../Empirical/include/emp/config/../tools/string_utils.hpp:35: ../../../Empirical/include/emp/config/../tools/../base/array.hpp:28:10: fatal error: '../../../third-party/cereal/include/cereal/cereal.hpp' file not found #include "../../../third-party/cereal/include/cereal/cereal.hpp" ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. make: *** [project_name] Error 1

Hi @bramvandijk88

Thanks for bringing this to our attention! We've updated the documentation to clarify a missing step of initializing the project subrepositories. If you do

git submodule update --init --recursive --depth 1 and then try compiling again it should hopefully work!

bramvandijk88 commented 6 months ago

If you do git submodule update --init --recursive --depth 1 and then try compiling again it should hopefully work!

Installation was running for ~ 10 minutes until I ran into this: 

rm -rf test*.out
cd hardware && make test
mkdir -p temp
c++ -std=c++20 -g -pthread -Wall -Wno-unused-function -Wno-unused-private-field -I../../include/ -I../../ -I../../third-party/cereal/include/ -DCATCH_CONFIG_MAIN event_driven_gp.cpp -o test-event_driven_gp.out
In file included from event_driven_gp.cpp:25:
In file included from ../../include/emp/hardware/EventDrivenGP.hpp:36:
In file included from ../../include/emp/hardware/../matching/MatchBin.hpp:36:
../../include/emp/hardware/../matching/../../../third-party/robin-hood-hashing/src/include/robin_hood.h:54:14: fatal error: 'sys/auxv.h' file not found
#    include <sys/auxv.h> // for getauxval
             ^~~~~~~~~~~~
1 error generated.
make[2]: *** [test-event_driven_gp] Error 1
make[1]: *** [test-hardware] Error 2
make: *** [test-native-regular] Error 2
bramvandijk88 commented 6 months ago

On a different note (I'm sure the installation will eventually work out), I'm happy @LTLA went into depth on the C stuff (e.g. the smart pointer comments), as it's been over 6 years since I did anything with C. That leaves room for me to test the examples thoroughly, which I am quite fond of doing. I hope that together with @LTLA's comments, we'll cover all the bases.

mmore500 commented 6 months ago

Hi @bramvandijk88 ---

Apologies for the friction here!! Fortunately, I think we just fixed this issue as @mercere99 just ran into it last week. (This PR https://github.com/devosoft/Empirical/pull/515).

The fix has been merged onto the master Empirical branch. It updates our git submodule of the robin-hood hashing repository. It can be a little involved to bump submodule versions on an existing clone, so the easiest thing might be to start a fresh clone of Empirical. When you clone now and initialize submodules, it will pull a version of robin-hood hashing that doesn't use sys/auxv.h.

Looking forward to getting your feedback on the examples, and please do let us know of any further issues you may run into.

bramvandijk88 commented 6 months ago

Hi!

I got through the installation now, and have completed the checklist. I have also tested some of the (basic) examples and tested some self-written code using WASM myself. Here's my review:

The authors clearly did all the due diligence when it comes to installation and automated testing. Barring a few minor hick-ups (which they resolved), everything installed smoothly and I got into the actual fun part. Similar to the other reviewer, I feel like the paper could have provided a bit more details. If anything, the current paper undersells the utility of this tool, which is definitely a wheel I am not going to reinvent in the future (in fact, I tagged some students to pick some of this up and build something cool with it).

I've always enjoyed explorable and sharable simulations, which is why I gradually moved from C/C++ to javascript/typescript. While I was made aware of Webassembly, getting all of this properly set up sounded like too much of a hassle. TLDR: the authors did the community a great service! I would say my only (minor) disappointment (which may be a misconception) is that running the code in a web browser still requires a server, meaning it will not be as easy for students that are just learning to code: e.g. they first need to learn how to install python and launch a local server using that. It's a minor annoyance I have when I am faced with 50 students all of which have their own errors in "trying to install stuff", but I suppose that's not always avoidable. It would be amazing if the authors considered to, in the future, porting their tool to something like WASMfiddle which would further enable educators such as myself to use their software in teaching students basic programming skills.

On the actual structures provided by Empirical, I did not have the time to test all the individual types (worlds, genomes) and their accompanying methods (GetNumOrgs, etc.). However, the provided examples in the "Built with Emperical Gallery" (all of which have a link to their original code) would give a starting coder plenty of room to work with. I am a little surprised by the performance of e.g. the cancer model still appearing quite slow, but perhaps there is a lot more going on under the hood than I realise. None of this invalidates the software anyway.

In other words, it seems like any enthusiast would get something running with relatively little effort. Congratulations to the authors for this amazing piece of software!

mahfuz05062 commented 6 months ago

@bramvandijk88 Thank you for completing the review! I see the other review is also going very well and I will check back later.

mahfuz05062 commented 6 months ago

@LTLA Please let us know if you have any questions or need any assistance on the remaining task of the checklist.

LTLA commented 6 months ago

Sorry for the late reply; lgtm. Some of my issues are still open but are trending in the right direction so not a blocker.

mmore500 commented 6 months ago

Pinging my co-maintainers and try to get a review on the de-jquerifyication PR that's open. Hopefully progress on that soon.

mahfuz05062 commented 5 months ago

@mmore500 Any update from your co-maintainers?

mmore500 commented 5 months ago

We are planning to merge the PR Monday (tomorrow) and I will follow up here then!

mmore500 commented 5 months ago

🎉 it’s done! I think we should be ready to move towards wrapping up

mmore500 commented 5 months ago

Closed the last reviewer issue on the tracker! Added the remaining sub-issue of emp::Ptr debug mode thread safety to the medium-term roadmap, as the thread safety issue arises specifically in the context of debug-mode telemetry as opposed to user production code.

Let me know if there’s other action we should take on our end to progress this review process!

mahfuz05062 commented 5 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

mahfuz05062 commented 5 months ago

Thanks @mmore500! Can you also complete the post-review tasks from above?

mmore500 commented 5 months ago

Additional Author Tasks After Review is Complete

mmore500 commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

mmore500 commented 5 months ago

Hi @mahfuz05062 --- I have completed the author tasks for after review is complete. The release version accompanying the paper is v1.1.1 and the Zenodo DOI is https://doi.org/10.5281/zenodo.11271122

mmore500 commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

mmore500 commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

mmore500 commented 5 months ago

@mahfuz05062 --- fixed a last reference in the manuscript, and updated the comment above to pin to v1.1.1

I think this should be all set on our end!

mahfuz05062 commented 5 months ago

Thanks @mmore500! I will go through my tasks and take the next steps

mahfuz05062 commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

mahfuz05062 commented 5 months ago

@editorialbot set 10.5281/zenodo.11271122 as archive

editorialbot commented 5 months ago

Done! archive is now 10.5281/zenodo.11271122

mahfuz05062 commented 5 months ago

@editorialbot set v1.1.1 as version

editorialbot commented 5 months ago

Done! version is now v1.1.1

mahfuz05062 commented 5 months ago

@editorialbot check references

editorialbot commented 5 months ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1145/3062341.3062363 is OK
- 10.1145/367701.367714 is OK
- 10.1145/2048147.2048224 is OK
- 10.1093/bib/bbp073 is OK
- 10.1186/1752-0509-6-96 is OK
- 10.1145/3205455.3205523 is OK
- 10.1162/artl_a_00284 is OK
- 10.1177/0037549712462620 is OK
- 10.1186/2194-3206-1-3 is OK
- 10.1145/3205651.3205780 is OK
- 10.5281/zenodo.4118608 is OK
- 10.1145/3185517 is OK
- 10.5281/zenodo.2575606 is OK
- 10.1038/nchem.1149 is OK
- 10.48550/ARXIV.2108.00382 is OK
- 10.48550/ARXIV.2211.10897 is OK
- 10.48550/ARXIV.2405.09389 is OK

MISSING DOIs

- No DOI given, and none found for title: Not so fast: Analyzing the performance of webassem...
- No DOI given, and none found for title: Squares: A Fast Counter-Based RNG
- No DOI given, and none found for title: LEARNING EVOLUTION AND THE NATURE OF SCIENCE USING...
- No DOI given, and none found for title: Netlogo: A simple environment for modeling complex...
- No DOI given, and none found for title: shiny: Web Application Framework for R
- No DOI given, and none found for title: shiny: Web Application Framework for R
- No DOI given, and none found for title: Suicide, signals, and symbionts: Evolving cooperat...
- No DOI given, and none found for title: Changing Environments Drive the Separation of Gene...
- No DOI given, and none found for title:  Open Science Philosophy

INVALID DOIs

- None
mahfuz05062 commented 5 months ago

@mmore500 I found several issues with the citation/reference. See below:

mmore500 commented 5 months ago

Thanks for pointing these out. Fixing, should just be a few minutes.

mmore500 commented 5 months ago

@editorialbot generate pdf

editorialbot commented 5 months ago

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

mahfuz05062 commented 5 months ago

@mmore500 - please change the title of your zenodo deposit to match the title of the paper. This is a change of metadata only, which doesn't require a new deposit or new DOI.

mmore500 commented 5 months ago

On it :+1:

Think I just fixed the references