Closed whedon closed 3 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @chrishavlin, @rougier, @phamvanvung 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:
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
Software report (experimental):
github.com/AlDanial/cloc v 1.88 T=0.30 s (770.1 files/s, 143351.4 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 104 6585 10275 17224
reStructuredText 69 1142 672 2182
CSS 3 22 5 1649
JSON 16 1 0 576
GLSL 11 158 78 370
Markdown 7 120 0 270
YAML 4 22 26 235
TeX 1 27 0 230
HTML 2 8 20 204
DOS Batch 2 16 1 131
Bourne Shell 3 18 17 101
PowerShell 3 15 19 85
JavaScript 2 12 1 68
make 1 7 6 25
INI 1 0 0 5
-------------------------------------------------------------------------------
SUM: 229 8153 11120 23355
-------------------------------------------------------------------------------
Statistical information for the repository '5e5b5f8f0b2021594ed1cb6d' was
gathered on 2021/06/18.
The following historical commit information, by author, was found:
Author Commits Insertions Deletions % of changes
Adam 1 1 0 0.00
Alexandre Gauvin 2 38 20 0.06
Aman Soni 1 1 1 0.00
Amit Chaudhari 7 94 45 0.15
Ariel Rokem 48 519 725 1.33
Bago Amirbekian 11 198 201 0.42
Bishakh Ghosh 3 52 26 0.08
CHrlS98 16 1032 602 1.74
ChenCheng0630 7 202 62 0.28
Christopher Nguyen 2 23 3 0.03
David Reagan 22 593 259 0.91
Devanshu Modi 1 16 16 0.03
Eleftherios Garyfall 458 13607 8301 23.34
Enes Albay 5 28 11 0.04
Etienne St-Onge 12 638 393 1.10
Filipi Nascimento Si 6 289 37 0.35
Gauvin Alexandre 1 3 0 0.00
Gottipati Gautam 2 6 6 0.01
Gregory R. Lee 5 34 21 0.06
Guillaume Favelier 4 160 13 0.18
Gurdit Siyan 1 13 8 0.02
Ian Nimmo-Smith 1 2 0 0.00
Javier Guaje 26 3517 2064 5.94
Jhalak Gupta 2 173 140 0.33
Jiri Borovec 2 6 23 0.03
Jon Haitz Legarreta 6 34 35 0.07
Karan 49 2496 404 3.09
Kesshi Jordan 11 420 205 0.67
Kevin Sitek 10 34 15 0.05
Lenix Lobo 47 1027 536 1.66
Liam Donohue 4 313 10 0.34
LoopThrough-i-j 1 149 0 0.16
MIHIR 1 86 165 0.27
Marc-Alexandre Côté 56 5087 3081 8.70
Marssis 2 14 14 0.03
Matthew Brett 17 223 588 0.86
Melina Raglin 52 1291 263 1.66
Naman Bansal 14 62 17 0.08
Nasim Anousheh 20 554 282 0.89
Omar Ocegueda 10 964 186 1.22
Pietro Astolfi 1 1 1 0.00
Prashil 26 379 246 0.67
Ranveer Aggarwal 72 3515 747 4.54
Sajag Swami 12 693 371 1.13
Samuel St-Jean 1 12 14 0.03
Sanjay Marreddi 9 359 131 0.52
Saransh Jain 16 420 105 0.56
Scott Trinkle 2 2 4 0.01
Serge Koudoro 443 16386 7287 25.22
Shahnawaz Ahmed 4 11 7 0.02
Shreyas Bhujbal 16 149 52 0.21
Soham Biswas 172 5126 2056 7.65
Stefan van der Walt 5 263 162 0.45
SunTzunami 6 232 10 0.26
Tingyi Wanyan 5 128 128 0.27
Tushar 15 328 55 0.41
Vivek Choudhary 22 693 193 0.94
Yaroslav Halchenko 1 3 2 0.01
antrikshmisri 11 142 35 0.19
devmessias 8 149 28 0.19
ganimtron-10 1 1 1 0.00
haran2001 2 16 16 0.03
ibrahimAnis 11 318 122 0.47
theaverageguy 3 3 3 0.01
wasserth 1 1 1 0.00
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
Aman Soni 1 100.0 4.2 0.00
Amit Chaudhari 81 86.2 2.5 3.70
Ariel Rokem 5 1.0 58.1 0.00
CHrlS98 698 67.6 4.1 16.05
ChenCheng0630 159 78.7 15.2 17.61
David Reagan 246 41.5 31.5 16.26
Devanshu Modi 16 100.0 14.5 0.00
Eleftherios Garyfall 3852 28.3 47.1 7.66
Enes Albay 17 60.7 36.8 11.76
Etienne St-Onge 265 41.5 52.5 7.92
Filipi Nascimento Si 247 85.5 15.5 17.00
Gauvin Alexandre 28 933.3 69.9 0.00
Gottipati Gautam 6 100.0 15.7 100.00
Gregory R. Lee 1 2.9 64.5 0.00
Guillaume Favelier 72 45.0 24.0 1.39
Gurdit Siyan 13 100.0 2.2 0.00
Javier Guaje 2023 57.5 13.0 9.99
Jhalak Gupta 12 6.9 2.2 0.00
Jiri Borovec 1 16.7 38.8 100.00
Jon Haitz Legarreta 33 97.1 20.8 0.00
Karan 1591 63.7 35.9 13.45
Kesshi Jordan 177 42.1 51.3 10.73
Kevin Sitek 6 17.6 27.0 16.67
Lenix Lobo 393 38.3 13.4 12.21
Liam Donohue 233 74.4 15.1 3.00
LoopThrough-i-j 149 100.0 1.1 15.44
MIHIR 86 100.0 1.9 34.88
Marc-Alexandre Côté 3042 59.8 39.3 10.49
Marssis 14 100.0 19.4 0.00
Melina Raglin 761 58.9 11.7 18.79
Naman Bansal 47 75.8 14.0 6.38
Nasim Anousheh 270 48.7 13.0 24.07
Pietro Astolfi 1 100.0 9.6 0.00
Prashil 111 29.3 28.5 2.70
Ranveer Aggarwal 1039 29.6 51.8 14.05
Sanjay Marreddi 227 63.2 7.4 24.23
Saransh Jain 280 66.7 15.7 24.64
Serge Koudoro 13088 79.9 25.6 12.85
Shreyas Bhujbal 98 65.8 15.1 3.06
Soham Biswas 3237 63.1 11.1 12.64
SunTzunami 507 218.5 3.5 18.34
Tushar 288 87.8 7.8 10.76
Vivek Choudhary 302 43.6 15.0 11.59
antrikshmisri 133 93.7 0.4 1.50
devmessias 123 82.6 1.5 3.25
ganimtron-10 1 100.0 4.4 0.00
haran2001 16 100.0 3.5 0.00
ibrahimAnis 168 52.8 14.7 10.71
wasserth 1 100.0 36.2 0.00
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.25080/majora-92bf1922-00a is OK
- 10.1109/mcse.2007.55 is OK
- 10.25080/majora-7b98e3ed-00e is OK
- 10.1109/mcse.2011.35 is OK
- 10.3389/fninf.2014.00008 is OK
- 10.1051/0004-6361/201629272 is OK
- 10.1051/0004-6361/201322068 is OK
- 10.1515/ntrev-2012-0043 is OK
- 10.1109/nmdc.2011.6155316 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@chrishavlin, @rougier, and @phamvanvung - Thanks for agreeing to review this submission. This is the review thread for the paper. All of our communications will happen here from now on.
Both reviewers have checklists at the top of this thread with the JOSS requirements. 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.
Please read the first couple of comments in this issue carefully, so that you can accept the invitation from JOSS and be able to check items, and so that you don't get overwhelmed with notifications from other activities in JOSS.
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#3384
so that a link is created to this thread (and I can keep an eye on what is happening). 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 either of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.
Some update on my (stalled) review: I have a hard time installing vtk on OSX Catalina. Once solved, I'll be able to resume my review. [EDIT] Solved.
Some update on my (stalled) review: I have a hard time installing vtk on OSX Catalina. Once solved, I'll be able to resume my review. [EDIT] Solved.
I am running macOS too (Big Sur) and tried installing the software (all three ways mentioned in the documentation: pip, conda, and development). They work well. It also downloads and installs vtk!=9.0.0,>=8.1.2 (from fury) automatically.
I've a hard time installing VTK on OSX Catalina, just for the record, the procedure using conda was:
conda config --set channel_priority strict
conda create -n vtk -c conda-forge python=3.7 numpy scipy pillow vtk
conda activate vtk
python -c "import vtk"
I had no trouble installing Fury following the instructions but the test failed with a segmentation fault (when testing picking). The output was rather scarce so it might be difficult to debug. But I managed to run some of the tutorials (I've not tested all of them). Overall it's a nice and much need package. Each example is a bit slow to start but I suspect this is because of VTK.
Concerning the paper, I've some minor comments:
I tested the software on: macOS (Big Sur), conda 4.10.2 environment. All the installation instructions work well.
There is an automated test, and I tried executing the test. Many cases showed "PASSED" status, but there are still some errors shown (this might be due to the version compatibility from my environment): E.g.: fury/tests/test_pick.py::test_picking_manager Fatal Python error: Segmentation fault
The statement of need is clear from the Software paper (PDF). However, though I can figure out the statement of needs from the Documentation page, it is unclear for me (as required from the checklist).
Given the limitation of the number of pages, the Software paper does a good job stating its aims, architectures, and quick pointers to comparable and commonly used software packages. They also provide the main differences between these similar packages and FURY. Interested readers can refer to these pointers for further comparisons if needed.
Regarding the references:
All in all, I have positive feelings regarding the usability of this software and highly support its use.
Thanks @phamvanvung - as much as possible, can you edit your comment above to make it clear where you think something needs to be fixed/changed before publication, and where things are suggested improvements that don't block you from checking off review criteria? Given that I think you've checked off all the review criteria, I assume none of your comments are blockers, correct?
And it any items are blockers, it you want to create issues in the source repository, you could (but don't have to).
:wave: @rougier, please update us on how your review is going (this is an automated reminder).
:wave: @chrishavlin, please update us on how your review is going (this is an automated reminder).
Thanks @phamvanvung - as much as possible, can you edit your comment above to make it clear where you think something needs to be fixed/changed before publication, and where things are suggested improvements that don't block you from checking off review criteria? Given that I think you've checked off all the review criteria, I assume none of your comments are blockers, correct?
And it any items are blockers, it you want to create issues in the source repository, you could (but don't have to).
Hi, For me, the automated test issue is minor and might due to version differences in my environment. I now changed the references to be a blocker and I think it should be easy and quick for the authors to fix.
@phamvanvung - Can you be specific (in the copy of the issue that you will create in https://github.com/fury-gl/fury) about what you think needs to be changed?
I'm waiting for author's feedback to update my review.
I'm waiting for author's feedback to update my review.
👋 @Garyfallidis ☝️
Thank you all greatly for your reviews. Will get back to you at the soonest possible.
Dear Prof. Rougier (@rougier), thank you for your review. The conda packages for vtk are not officially provided by Kitware and therefore we suggest using pip directly to install vtk.
I had no trouble installing Fury following the instructions but the test failed with a segmentation fault (when testing picking).
This is probably because of the conda installation. Time permitted, please uninstall conda vtk and upgrade vtk using pip.
The output was rather scarce so it might be difficult to debug. But I managed to run some of the tutorials (I've not tested all of them). Overall it's a nice and much needed package.
Thank you for your encouraging words.
Each example is a bit slow to start but I suspect this is because of VTK.
We should be able to partially import functions with the latest versions of VTK and hopefully reduce starting time. On my desktop, the starting time for the viz_selection.py example was 7 seconds (i7 CPU @ 3.40GHz and NVIDIA GeForce GTX 1070).
Concerning the paper, I've some minor comments: The paper is rather short which is ok for JOSS but in the meantime it's a bit frustrating for the reader. For example, you briefly compared with mayavi/glumpy/vispy but it's a bit difficult to compare since you do not provide much details. For example, why did you not reuse one of them (lack of support? some ingredients were missing, etc).
Great question. VTK was providing some utilities that were hard to implement and not available in the other packages. Nonetheless, I started a project with direct access to OpenGL back in 2008 and I found it hard to scale on academic resources. However, we should say here that all 3 projects mayavi/glumpy and vispy have been an inspiration for FURY and the decision to go with VTK was a multi-year process moving back and forth through different libraries and standards. In the end we really liked the idea of updating existing shaders that VTK provides and we had quite a bit of code from DIPY (using VTK) that we could reuse in FURY.
You explain that FURY is interoperable with a lot of software but we don't have details. For example, what does it mean for matplotlib? I think people (including me) would be very interested to have either one example or further explanations.
FURY was built to be fully compatible with the data structures provided by numpy, this makes the library compatible with all the functions in Scipy and as some processing done in Pandas, TensorFlow, and Pytorch can output numpy arrays, then that information can be visualized in FURY too. This decision has also allowed developers to visualize the results of experiments obtained from NetworkX or PyBullet. Furthermore, some functionalities of FURY inherit/draw power from structures/functions in Matplotlib and Pillow. For instance, we have a function matplotlib_figure_to_numpy that allows us to grab a figure from matplotlib and show it in the 3D world with FURY. We would love to have the option to directly draw on a Matplotlib figure but this is still not implemented.
In the list of authors, it's nice to recognize "FURY Contributors". I just wondered if you could add either a footnote or a dedicated section to list them but this would imply permission and may be too much trouble.
We do provide the following page in our website that gives credit to all contributors. https://fury.gl/latest/community.html This page is using GitHub API and updates the current list of contributors in each website update (for every new release). Let me know if linking to this page is adequate.
For the DOI, I think some are missing (for example Numpy), could you fix them?
Yes, this is done.
I tested the software on: macOS (Big Sur), conda 4.10.2 environment. All the installation instructions work well.
Thank you for your review @phamvanvung and happy to hear that the instructions worked well for you.
There is an automated test, and I tried executing the test. Many cases showed "PASSED" status, but there are still some errors shown (this might be due to the version compatibility from my environment): E.g.: fury/tests/test_pick.py::test_picking_manager Fatal Python error: Segmentation fault
Yes, this looks like a conda issue. Please try to uninstall conda vtk and use the pip installer for vtk. Hopefully, this change will fix the issue.
The statement of need is clear from the Software paper (PDF). However, though I can figure out the statement of needs from the Documentation page, it is unclear for me (as required from the checklist).
Apologies, we are currently transitioning to an updated (new) website. We added the statement of need in the about section of the website. Please see merged PR here https://github.com/fury-gl/fury/pull/456
Given the limitation of the number of pages, the Software paper does a good job stating its aims, architectures, and quick pointers to comparable and commonly used software packages. They also provide the main differences between these similar packages and FURY. Interested readers can refer to these pointers for further comparisons if needed.
Thank you greatly for your positive feedback.
Regarding the references:
It was hard for me to find the source for the "Bullet physics library" reference. Please add "Available at: http://www.bulletphysics.org," if possible. Here is the DOI for the "Array programming with NumPy" reference: https://doi.org/10.1038/s41586-020-2649-2 Here is the DOI for the "Advancing Education and Research in Nanotechnology" reference: http://dx.doi.org/10.1109/MCSE.2008.120 Similarly, the authors can try and search for the DOIs of the missing references. All in all, I have positive feelings regarding the usability of this software and highly support its use.
Thank you again @phamvanvung . We corrected the links and DOIs as suggested in FURY PRs (453-455) https://github.com/fury-gl/fury/pull/453 https://github.com/fury-gl/fury/pull/454 https://github.com/fury-gl/fury/pull/455
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Dear @danielskatz we answered the reviewers we will let you notify them. Thank you in advance.
The reviewers should see your changes here.
@Garyfallidis Thanks for all the explanations and the nice package. I hope we'll have some opportunities to interact / talk about 3D visualization. @danielskatz I'm done with the review and all my comments have been addressed. For me the paper can be published.
Thanks @rougier!
👋 @phamvanvung - seeing that you have checked off all your items, are you also supportive of publishing the work?
It would be a pleasure and an honor to talk to you @rougier. Your books, code and work have been an inspiration for myself and many members of the FURY team.
@Garyfallidis I'll contact you by mail then.
👋 @phamvanvung - seeing that you have checked off all your items, are you also supportive of publishing the work?
Sorry for the late reply, yes, as from my review and the checklist. I recommend publishing this work.
Thanks @phamvanvung
👋 @chrishavlin - how is your review coming along?
Dear @danielskatz it seems @chrishavlin has not responded for quite some time. Any suggestions for moving forward? The other two reviewers seem happy.
I'll email @chrishavlin
Thank you @danielskatz
Apologies for the slow reply! I just updated my checklist and I'm happy to recommend publishing the work. A very useful contribution, I look forward to using FURY myself in future work.
Great to hear @chrishavlin! Thank you for your review.
@Garyfallidis - the next step is for me to proofread the paper, but I'm actually on vacation this week, so I will get it done by early next week at the latest.
Sounds good @danielskatz
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@Garyfallidis - in the paper, I don't understand the sentence in the Rendering Engine paragraph that starts with "Furthermore," Is this talking about your software? Why "only"?
I think "Mayavi however FURY" should be "Mayavi. FURY, however,"
In the references, you probably need to protect some casing with {}s - the Harris reference should probably say "CUDA" and "SIGGARAPH" There may be others like this, though I don't see any immediately.
👋 @Garyfallidis - please let me know when you have made your changes.
Dear @danielskatz your changes have been addressed in this merged pull request https://github.com/fury-gl/fury/pull/464
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
I think two more small changes are needed - see https://github.com/fury-gl/fury/pull/466
Done and merged. Apologies we missed that.
@whedon generate pdf
Submitting author: @garyfallidis (Eleftherios Garyfallidis) Repository: https://github.com/fury-gl/fury Version: v0.7.1 Editor: @danielskatz Reviewer: @chrishavlin, @rougier, @phamvanvung Archive: 10.5281/zenodo.5160945
:warning: JOSS reduced service mode :warning:
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
Status
Status badge code:
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
@chrishavlin & @rougier & @phamvanvung, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @danielskatz 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 @chrishavlin
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @rougier
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @phamvanvung
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper