search

openjournals / joss-reviews

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

[REVIEW]: NeuralFieldEq.jl: A flexible solver to compute Neural Field Equations in several scenarios #3974

Closed whedon closed 2 years ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@tiagoseq<!--end-author-handle-- (Tiago Sequeira) Repository: https://github.com/tiagoseq/NeuralFieldEq.jl Branch with paper.md (empty if default branch): Version: v0.1.3 Editor: !--editor-->@jarvist<!--end-editor-- Reviewers: @rougier, @gdetor Archive: 10.5281/zenodo.6226695

: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/85354f0f2b6244439e90d51fe28a9ef5"><img src="https://joss.theoj.org/papers/85354f0f2b6244439e90d51fe28a9ef5/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/85354f0f2b6244439e90d51fe28a9ef5/status.svg)](https://joss.theoj.org/papers/85354f0f2b6244439e90d51fe28a9ef5)

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

@rougier & @gdetor, 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 @jarvist 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 @rougier

✨ 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 @gdetor

✨ 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 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @rougier, @gdetor 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 2 years ago

Wordcount for paper.md is 1046

whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.16 s (251.7 files/s, 117203.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
CSS                              2            701             49          14825
Julia                           14            190            158            845
Markdown                         7            141              0            538
JavaScript                       5             44             78            511
HTML                             5             34              0            233
TeX                              1             13              0            121
YAML                             3              0              1            114
TOML                             2              5              0             20
JSON                             1              0              0              7
-------------------------------------------------------------------------------
SUM:                            40           1128            286          17214
-------------------------------------------------------------------------------

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

Author                     Commits    Insertions      Deletions    % of changes
tiagoseq                         1           633              0          100.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
tiagoseq                    633          100.0          0.0               12.32
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1113/jphysiol.1952.sp004764 is OK
- 10.1109/JPROC.2004.840301 is OK
- 10.1002/cne.21974 is OK
- 10.1016/S0006-3495(72)86068-5 is OK
- 10.1103/PhysRevE.82.055701 is OK
- 10.1007/978-3-642-54593-1_6 is OK
- 10.3389/fninf.2015.00025 is OK
- 10.1186/2190-8567-4-1 is OK
- 10.1007/BF00337259 is OK
- 10.1137/141000671 is OK
- 10.1109/ICSTCC.2019.8885972 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 2 years ago

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

jarvist commented 2 years ago

Dear @rougier and @gdetor, how are things coming along? I think your invitation link may have expired @gdetor.

jarvist commented 2 years ago

@whedon re-invite @gdetor as reviewer

Just in case!

whedon commented 2 years ago

The reviewer already has a pending invite.

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

jarvist commented 2 years ago

OK! Thanks @whedon.

whedon commented 2 years ago

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

whedon commented 2 years ago

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

rougier commented 2 years ago

Here are some temporary comments on the submission since I'm waiting for an issue to be solved before going further.

  1. Repository is way too big (36Mo) due mostly to the presence of gif animations in the imag and doc directories. Problem is that the doc directories containes the build website for no obvious reason. I would advise to cleanup the repository. But problem will remain because the build addition has been commited and the repository will stay big unless the git history is rewritten. Another solution would be to create a new repository and make sure to only add necessary stuff.

  2. The name of the repository is a bit weird with the .jl extension but I don't know if it is usage for Julia packages.

  3. Since computation is a bit long, would it be possible to have some kind of progress indicator when it is ran?

  4. The summary in the paper needs to be worked a bit. Especially the first sentence that seems to be missing a verb. But more generally, I think i would be ok to dive directly in the neural field, starting at "Wilson & Cowan (1972)...".

  5. Equation 1 seems to be missing the axonal speed term. This is ok since it does not appear in the original equation but you shoudl rephrase the paragraph to explain that speed can be introduced in the equation and then rewrite the full equation.

  6. "when the coupling is positive (negative) the synapses are excitatory (inhibitory)" -> "when the coupling is positive (resp. negative), the synapses are excitatory (resp. inhibitory)

  7. "Although the this" -> "Although this"

  8. Overall, I think the paper could be improved because there are some typos that can easily be fixed and the package usage section should be improved to better introduce the package. As it is currently written it's a bit hard to follow author's explanations.

tiagoseq commented 2 years ago

Hello, I can answer some of the comments:

  1. Indeed the repo is too big due to the animations present in the docs section. I'll see what I can do to solve this problem, I guess I will take your advice and create a new repository for the docs, or re-write "lighter" documentation without too many animations. I have to address this question carefully, I'll give updates on this soon.

  2. The name of the repository follows the julia guidelines about package names, that's why the extension .jl appears. For example, these packages have the extension: DifferentialEquations.jl and FFTW.jl.

  3. I guess that is possible to add a progress indicator, I'll give updates soon.

  4. to 7. Since these issues are easier to fix, I'll check them first. I agree with you, the summary needs to be written more carefully.

  5. Ok, I will read carefully again the package usage section and try to improve it to make it more clear.

When I have some of these points done, I'll write here.

Thanks, Tiago

gdetor commented 2 years ago

Here are my comments on the manuscript. I'll provide my comments on the source code later.

Text Comments

  1. Memory, perception, and other functions of the human brain do not occur only in the cerebral cortex. Please rephrase the sentences in lines 7 - 10.

  2. It would be better to rewrite equation 1 in a more general way. For instance, the kernel function K(x, t) is not always isotropic or homogeneous. You could incorporate the delay term as well. Why is the norm a Euclidean one (is the only supported norm)? The integral in equation 1 is always two-dimensional (please replace the square with d in the differential of the integral). You could also use a different symbol for indicating the dimension. What about 3-dimensional neural fields?

  3. You could consider citing these two references: Coombes, "Waves, bumps, and patterns in neural field theories", 2005 and Bressloff, "Spatiotemporal dynamics of continuum neural fields", 2011. Thus, readers who are not experts in neural fields can get a general idea.

  4. Could you please provide a reference regarding the performance of Julia? See lines 40-42 in the main text.

  5. The first paragraph in the "Package usage" section is obscure. This section needs some improvements.

  6. The sentence in lines 44-46 is not clear.

  7. Could you provide more details regarding the performance of the code?

Language

The text needs polishing. Here are some typos I spotted in the main text:

  1. Please remove "that" from the very first sentence of the summary.
  2. Line 30, "Although the this ..." -> "Although this ..."
  3. Lines 35-36, "the scope of the master's thesis [Ref] ..." -> "the scope of [Ref]"
  4. Line 42, "the the" -> "the"
  5. Line 44, "non-delayed or delayed equations" -> "non-delayed or delayed"
  6. Line 58, "access it at the previously" -> "access it at previously"
tiagoseq commented 2 years ago

Hello, Thank you for your suggestions, generally, I agree with them. Over the next weekend I'll look more carefully to these suggestions and implement them. For now I just fixed the typos.

When I've updated I'll communicate.

Thanks!

jarvist commented 2 years ago

Dear @tiagoseq , How is it coming along?

tiagoseq commented 2 years ago

Hello, Sorry for the delay, unfortunately, only yesterday was possible for me to address the comments pointed out. I'm planning to reply to it during this week.

I'll give updates soon

tiagoseq commented 2 years ago

Hello, I've released a new major version of the manuscript, with the following changes:

Now, answering some questions: @rougier

  1. Regarding the repo size, i've checked the settings of the repo on github and says that is about 10 Mb. Am I seeing this wrong? Yet, I have to work in this issue;
  2. I thought that add a status bar would be straightforward, but it isn't. Probably, next week I have to check it out.

@gdetor

  1. I wrote an isotropic kernel because the solver do not support (yet) such functions. In this method K is homogeneous and isotropic (check line 15);
  2. Yes, the euclidean norm is the only supported, the method implemented only considers p-2 norms;
  3. I did not addressed the 3D case, because the first motivation of the solver was to study fields that exhibit working memory mechanisms applied to cognitive robotics (first paragraph of Statement of need) and there was no need to have one more dimension.

Thank you, and I'll be grateful when you can give feedback, Tiago

rougier commented 2 years ago

@tiagoseq

You can check the size locally with command du -sh NeuralFieldEq.jl (Linux or OSX). I think size reported by GitHub is only for current files, without the history of commits which can be quite large.

For the progress bar, it's only a suggestion, nothing really important. There is certainly a Julia package to do just that. If it's the case that would be nice and easy to add it.

gdetor commented 2 years ago

@tiagoseq @jarvist Here are my comments on the Source Code:

  1. Many parts of the code are not documented (for instance the functions in script AuxScripts.jl or in SolveStruct.jl). Please document all the functions and variables within the code.
  2. Please provide a list with the contributors (on the Github repository as a section in the README file)
  3. Please add community guidelines (especially for issues and bugs) in the repo's README file.

Other than that everything looks fine to me. The installation instructions are OK. The code works properly. The performance of the package is now reported in the main text. The repository contains automated tests and all tests pass with no any apparent issues.

My previous comments regarding the main text have been addressed by the author.

@tiagoseq Could you please generate the new pdf article proof?

tiagoseq commented 2 years ago

@gdetor

The new version of the manuscript is generated.

@rougier I will move the docs to an independent repository and clean the git history to reduce the size of the repo. I'll look this issue after wednesday. Thank you for this suggestion! Yes, there is a Julia package to implement a status bar, probably is straight forward. I like that nice-to-have feature, I'll try to implement it after addressing the major issues pointed out!

Thank you!

tiagoseq commented 2 years ago

@whedon generate pdf

whedon commented 2 years ago

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

rougier commented 2 years ago

@tiagoseq PDF has been rebuilt, does that mean you adressed our comments with @gdetor?

tiagoseq commented 2 years ago

Hello, Sorry for the delay, today I finally had time to work on this issue. @rougier yes, I rebuilt the PDF with the comments that you and @gdetor made to the manuscript. Every comment, suggestion was addressed in this new version.

As for the code, what is already done is:

What I'll do next @rougier :

When the new version is released, I'll communicate here

tiagoseq commented 2 years ago

Hello, A pull request with a new version with the following features has been made:

I'm waiting for the auto-merge, made by JuliaRegistrator autobot. When the new version (v0.1.3) is released I'll let you know.

rougier commented 2 years ago

Any progress?

tiagoseq commented 2 years ago

Hello, Yes, the new version of the package has been released. To update, please, go to julia terminal and type ] update. One can see the new documentation and the status bar, while executing the function solveNFE.

However, the package is still too big. Even after moving the documentation to an independent repository, and after the creation of a new orphan branch and deleting the old master branch and commit history.

I have to check this question carefully. The problem relies on .git folder. I've tried another approaches but without success. Probably tomorrow I'll give some updates.

Nevertheless, the new version of the package can be checked after updating the local julia registries. If you have any issue updating the version 0.1.3 tell me.

Thank you!

rougier commented 2 years ago

Thanks for the update. For the size of the repository, I think it's ok. The .git folder probably carry the whole history and this might explain the size. For future code, make sure to not add all the files blindly or the problem will reappear. I've checked the new PDF and it now looks good to me. @jarvist @gdetor Looks good to me.

tiagoseq commented 2 years ago

Yes, learned my lesson. However, I still want to overcome this issue, to future downloads not to be so heavy. Thank you for your advice to add a progress bar @rougier. If i have any update about the commit history I'll let you know.

gdetor commented 2 years ago

@tiagoseq Thank you for the update. @jarvist @rougier Both the manuscript and the source code repository look good to me. All my comments have been addressed.

tiagoseq commented 2 years ago

Hi @gdetor thank you for your help in improvig the manuscript and the source code!

In order to move on with this process, I think you must complete your check list, as @rougier did, at the top of this thread.

Thanks, Tiago

gdetor commented 2 years ago

@jarvist @tiagoseq It seems I cannot access the check list. Any help?

tiagoseq commented 2 years ago

I searched for an answer in JOSS readthedocs and didn't see anything with respect on how complete the checklist.

@jarvist can you give any help? Or maybe @rougier could give some insight on how complete the check list

tiagoseq commented 2 years ago

Hello @jarvist, any help to @gdetor on the issue of filling out the task list? Just like @rougier already did

Thanks!

tiagoseq commented 2 years ago

@jarvist @tiagoseq It seems I cannot access the check list. Any help?

Hi @gdetor , just to check, when you say you cannot access the check list in the beginning of this thread, you're saying that you could not edit the comment that contains the checklist, right? It seems that @rougier didn't have any problem with this: image

I can't edit the edit the check list either, because probably only the reviewers can. When reading the initial comments, I found the following post:

The reviewer already has a pending invite.

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

Did you accepted this invitation? If not, probably this is the problem. If you accepted it, in order to be a reviewer then I guess only @jarvist can help us.

Thank you

gdetor commented 2 years ago

@tiagoseq That invitation has expired. So that might be the problem.
@jarvist Could you please send a new invitation? Thanks

tiagoseq commented 2 years ago

whedon is the bot that can re-invite you. I'll try to do call him up, but probably @jarvist is the only one that can do it

@whedon  re-invite @gdetor  as reviewer

tiagoseq commented 2 years ago

Well, no success in calling whedon to re-invite you. It must be @jarvist

danielskatz commented 2 years ago

bot commands need to be the first thing in a comment, but also, @whedon has been replaced by @editorialbot, and our method for generating and using checklists has changed. In this case, and for new reviews going forward, the reviewer (@gdetor) should put @editorialbot generate my checklist at the start of a new comment to get a checklist to use. This is a bit confusing here, since this review started under the old system.

gdetor commented 2 years ago

@editorialbot generate my checklist

tiagoseq commented 2 years ago

Hi @gdetor ,were you successful in completing the checklist?

@danielskatz Thank you for your help! So, if the review started under the old system, what does it means in practice? That the other reviewer, @rougier, has to generate a new checklist with the @editorialbot ? Or it is ok to proceed the process with one reviewer with the "old" checklist and the other one with the checklist generated with the @editorialbot ?

danielskatz commented 2 years ago

This can stay as is, with mixed checklists.

@gdetor might want to try the command again, as GitHub was dropping some requests for a bit yesterday.

gdetor commented 2 years ago

Review checklist for @gdetor

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

gdetor commented 2 years ago

@danielskatz Thank you for the instructions. @tiagoseq Thank you, now it should be OK.

tiagoseq commented 2 years ago

Thank you @gdetor !

@danielskatz , now that both checklists are completed, what is the next step of the process? Is there anything I can do?

danielskatz commented 2 years ago

@jarvist is the editor, so this should be directed to him.

tiagoseq commented 2 years ago

Hello, It seems @jarvist is absent, does not participate in this thread for a long time.

@danielskatz is there anything i can do in this case?

jarvist commented 2 years ago

Sorry for my absence! It was a busy end of term anyway, and then I was ill & off work for a while.

jarvist commented 2 years ago

@editorialbot generate pdf