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]: NLSIG-COVID19Lab #3002

Closed whedon closed 3 years ago

whedon commented 3 years ago

Submitting author: @somefunagba (Oluwasegun Somefun) Repository: https://github.com/somefunAgba/NLSIG-COVID19Lab Version: 2.1.1 Editor: @majensen Reviewer: @agahkarakuzu, @kakearney Archive: 10.5281/zenodo.4662275

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

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

@agahkarakuzu & @kakearney, 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 @majensen 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 @agahkarakuzu

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @kakearney

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. @agahkarakuzu, @kakearney 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

: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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.dib.2020.105683 is OK
- 10.1101/2020.04.13.20063354 is OK
- 10.1016/j.ejcon.2018.12.001 is OK
- 10.1007/978-0-85729-115-8_6 is OK
- 10.13140/RG.2.2.22509.95202 is OK
- 10.1101/2020.08.31.20185165 is OK
- 10.1063/1.3606555 is OK
- 10.1109/72.279181 is OK
- 10.1016/S1474-6670(17)45445-0 is OK
- 10.1016/j.ijnaoe.2018.04.002 is OK
- 10.1016/j.ejcon.2018.09.011 is OK
- 10.1109/ISCAS.2008.4541553 is OK
- 10.1109/IECON.1992.254474 is OK
- 10.1186/s12916-019-1406-6 is OK
- 10.1101/2020.07.03.20145672 is OK
- 10.1049/ic:19990713 is OK
- 10.1016/j.automatica.2006.07.018 is OK
- 10.1109/CDC.2003.1271825 is OK
- 10.1007/BF02551274 is OK
- 10.1017/atsip.2013.9 is OK
- 10.1177/1729881417703568 is OK
- 10.1007/978-3-642-36986-5_10 is OK
- 10.1038/s42256-020-00257-z is OK
- 10.1109/TNNLS.2016.2582924 is OK
- 10.1016/j.jtbi.2005.11.026 is OK
- 10.1038/35016072 is OK
- 10.1007/3-540-59497-3_175 is OK
- 10.1109/ICCV.2015.123 is OK
- 10.1162/neco.1997.9.8.1735 is OK
- 10.3201/eid1201.050396 is OK
- 10.1142/9789814261265_0009 is OK
- 10.1007/1-84628-259-4_10 is OK
- 10.1016/0005-1098(93)90052-U is OK
- 10.1007/1-84628-259-4 is OK
- 10.1016/j.conengprac.2007.03.010 is OK
- 10.1007/978-1-4842-2845-6 is OK
- 10.1109/IJCNN.2017.7966168 is OK
- 10.1007/3-540-49430-8_2 is OK
- 10.1371/journal.pone.0236860 is OK
- 10.1080/02664760903093633 is OK
- 10.1016/j.ecolmodel.2005.05.024 is OK
- 10.1063/1.4954543 is OK
- 10.1152/ajpheart.00219.2006 is OK
- 10.1109/ISTC.2014.6955084 is OK
- 10.1007/978-1-4899-7983-4 is OK
- 10.1109/JPROC.2020.2991885 is OK
- 10.3390/e21070627 is OK
- 10.3390/math8071174 is OK
- 10.1515/jee-2017-0069 is OK
- 10.1371/journal.pcbi.1002592 is OK
- 10.1016/S1474-6670(17)52049-2 is OK
- 10.1016/S1474-6670(17)58855-2 is OK
- 10.1155/2018/4231647 is OK
- 10.5772/60063 is OK
- 10.2307/2341367 is OK
- 10.1007/s13748-020-00218-y is OK
- 10.1109/ReCoSoC.2013.6581545 is OK
- 10.1016/j.neunet.2014.09.003 is OK
- 10.1073/pnas.1907373117 is OK
- 10.7641/CTA.2014.31117 is OK
- 10.7763/IJCTE.2009.V1.13 is OK
- 10.1016/j.ijid.2020.04.085 is OK
- 10.1080/10511979808965879 is OK
- 10.1049/cp:19991129 is OK
- 10.1109/JPROC.2017.2761740 is OK
- 10.1186/1742-4682-2-14 is OK
- 10.1007/s10462-011-9294-y is OK
- 10.1080/00031305.2017.1380080 is OK
- 10.1016/j.spl.2011.11.012 is OK
- 10.1109/5.58337 is OK
- 10.1007/s11071-020-05862-6 is OK
- 10.1109/IJCNN.2018.8489043 is OK
- 10.1016/j.jtbi.2012.07.024 is OK
- 10.1109/APEC.2016.7468308 is OK
- 10.1093/aob/mcg029 is OK
- 10.1109/TVLSI.2012.2232321 is OK

MISSING DOIs

- 10.1016/0005-1098(86)90095-6 may be a valid DOI for title: Computer-Controlled Systems: Theory and Design
- 10.1016/0005-1098(93)90084-7 may be a valid DOI for title: Linear Controller Design: Limits of Performance
- 10.1561/9781680833294 may be a valid DOI for title: Multi-Period Trading via Convex Optimization
- 10.1201/9781315219165 may be a valid DOI for title: Feedback, Nonlinear, and Distributed Circuits
- 10.9790/4200-0702012633 may be a valid DOI for title: Performance Analysis of the Sigmoid and Fibonacci Activation Functions in NGA Architecture for a Generalized Independent Component Analysis
- 10.1007/978-1-4020-6949-9 may be a valid DOI for title: Handbook of Continued Fractions for Special Functions
- 10.1007/978-3-319-09330-7_7 may be a valid DOI for title: Training Deep Fourier Neural Networks To Fit Time-Series Data
- 10.1109/cvpr.2016.90 may be a valid DOI for title: Deep Residual Learning for Image Recognition
- 10.1109/ism.2016.0052 may be a valid DOI for title: Fair and Efficient Bandwidth Allocation for Video Flows Using Sigmoidal Programming
- 10.3390/info6030432 may be a valid DOI for title: Sliding-Mode Speed Control of PMSM with Fuzzy-Logic Chattering Minimization—Design and Implementation
- 10.23943/princeton/9780691197296.001.0001 may be a valid DOI for title: Statistical Inference via Convex Optimization
- 10.21468/scipostphys.9.4.053 may be a valid DOI for title: Neural Network-Based Approach to Phase Space Integration
- 10.1109/tnnls.2013.2281217 may be a valid DOI for title: Local Stability Analysis of Discrete-Time, Continuous-State, Complex-Valued Recurrent Neural Networks with Inner State Feedback
- 10.3390/books978-3-03897-522-9 may be a valid DOI for title: Biological and Biogenic Crystallization
- 10.1613/jair.251 may be a valid DOI for title: Mean Field Theory for Sigmoid Belief Networks
- 10.1007/978-981-13-1298-4_1 may be a valid DOI for title: Intelligent Adaptive Fuzzy Control
- 10.1109/icdar.2003.1227801 may be a valid DOI for title: Best Practices for Convolutional Neural Networks Applied to Visual Document Analysis
- 10.1016/j.physa.2010.01.035 may be a valid DOI for title: Improvement of Signal-to-Noise Ratio by Stochastic Resonance in Sigmoid Function Threshold Systems, Demonstrated Using a CMOS Inverter
- 10.46945/bpj.10.1.03.01 may be a valid DOI for title: WHO Coronavirus Disease (COVID-19) Dashboard

INVALID DOIs

- None
whedon commented 3 years ago

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

whedon commented 3 years ago

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

agahkarakuzu commented 3 years ago

I gave this one a quick try (R2019b) after installing NLSIGCOVID19Lab as a Matlab app (applet), I could not find instructions regarding how to use GUI, so intuitively I tried clicking the model button:

Error using writetable (line 152)
Invalid parameter name: WriteMode.

Error in save_metrics (line 32)
    writetable(Tm, metricsfile,"Sheet",query_ccode,"WriteMode","overwritesheet");

Error in cov19_nlsigquery_applet (line 248)
save_metrics(focus,new_mts,country_code,time_data);

Error in nlsigcovid19lab/ModelButtonPushed (line 209)
                    cov19_nlsigquery_applet(search_code,update, ...

Error using matlab.ui.control.internal.controller.ComponentController/executeUserCallback (line 335)
Error while evaluating Button PrivateButtonPushedFcn.

Then I tried Update Database first, which threw me another error:

Error using upd_cdata_applet (line 294)

Oops! Something went wrong. Beyond my control.

Error in get_cdata_applet (line 40)
upd_cdata_applet(app, update, search_ccode);

Error in allcc_upd_applet (line 4)
[~,~,~] = get_cdata_applet(app,"ALL",1);

Error in nlsigcovid19lab/UpdateDatabaseButtonPushed (line 277)
            allcc_upd_applet(app);

Error using matlab.ui.control.internal.controller.ComponentController/executeUserCallback (line 335)
Error while evaluating Button PrivateButtonPushedFcn.

As the error message was not informative, I could not infer what to do next: https://github.com/somefunAgba/NLSIG-COVID19Lab/blob/2a99f46ae19ae1b1433816228353fcb7739ce45f/cvmain/cvcore/upd_cdata_applet.m#L291-L292

I guess a good first step would be having a really simple "getting started with GUI" documentation. If there is, can you please point me to that?

I also noticed that the tests are missing, which is another important assessment criteria. For quite a while, hooking MATLAB CI tests to a GitHub repo was not an option, but now through Azure, it is easily possible. There is a free license dedicated for providing public repos with CI. This repo uses the new app designer, which is a big advantage for testing GUI functionality as well. I think adding tests would be a second good step. I am happy to help out with Azure and code coverage integration when the tests are ready.

somefunAgba commented 3 years ago

Thanks @agahkarakuzu. I just tested the applet twice on my end in R2020b, and no errors. The applet was written in R2020a. There are internal catch blocks to ensure errors and exceptions will rarely occur and are rarely shown to the user, other things being equal.

Debugging the errors: One.

Error using writetable (line 152)
Invalid parameter name: WriteMode.

SOLUTION: I think the problem lies with string quotes which I used when writing the code. It seems 2019b does not implicitly perform a conversion between arguments specified with character quotes, and those with string quotes. I will fix that to ensure compatibility with previous MATLAB versions.

Two.

Error using upd_cdata_applet (line 294)
Oops! Something went wrong. Beyond my control.

SOLUTION: I think the cause here, may also be what triggered the first error above. The production code is structured, to be in a way, such that any exception in the _upd_cdataapplet file is returned as line 294. I will fix that as indicated above and also ensure the ME exceptions are printed also.

Other Issues: Thanks, I will create a separate file for the getting started with GUI documentation . There is one in the README.md file, but given this, I will enhance it to be more useful

Finally, about the missing MATLAB unit tests, CI integrations through Azure. I will get myself acquainted as soon as possible. I will inform you when these changes are made.

agahkarakuzu commented 3 years ago

Thank you so much for the quick response @somefunAgba!

I think these initial simple fixes would give this codebase a good usability boost. I have no expertise on the scientific aspect of this work, but will do my best to improve user experience by playing the annoying user wherever possible to ensure that the expected outcomes find the user :)

RE getting started docs, I would even briefly explain how to add your applet as an app to MATLAB as the first (or second) step, this convention is rather new and most matlab users are not accustomed to it.

Maybe using the wiki pages of your repo for documentation would be a good idea for the sake of content organization. Given that you already have figures etc uploaded, you can easily write up some informative markdowns.

somefunAgba commented 3 years ago

Thank you. Will factor in your suggestions

kakearney commented 3 years ago

I did some brief experimenting with this code, but so far have hit a few different problems that are preventing any in-depth analysis.

Issue 1

First, I encountered the same errors that @agahkarakuzu mentioned above when first opening the GUI and clicking the "Model" button without changing any of the default options. It sounds like that problem was diagnosed, so I'll await an update with that fix.

Issue 2

My second test was to open the GUI and try to change the country code. I changed the "Search Code" dropdown menu to "US" and received the following error:

Error in get_cdata_applet (line 106)
    sheets = sheetnames(cbc_CV19datafiled);

Error in getcasesbycc_applet (line 15)
[T,status,ccs] = get_cdata_applet(app, country_code, update);

Error in nlsigcovid19lab/SearchCodesDropDownValueChanged (line 294)
            t = getcasesbycc_applet(app,string(value),0,'o');

Error using matlab.ui.control.internal.controller.ComponentController/executeUserCallback (line 378)
Error while evaluating DropDown PrivateValueChangedFcn.

I also received this error when attempting to run the query_single script from the command line. Because most calculations are wrapped in try/catch blocks, I was unable to debug this further.

Issue 3

Insidious clear all, close all, and clc usage. Many of the primary scripts and functions include these statements, including the setup script (su.m) and all the main query scripts (query_single, query_batch, query_all). There is absolutely no reason for this and it can infuriate users like me when you dare to not just modify but completely wipe out their workspace. In general, I will not use code that modifies my existing workspace without warning (and that includes touching my exisiting workspace variables, command line history, or existing figures). I commented out the lines where I found them, but still found things being cleared and closed whenever I tried to test run a script, so I assume these calls are buried in many of the lower-level functions as well. I cannot continue testing until these are all removed. If you need a clear dedicated workspace, move the calculations to a function. If the calculations are already in a function, then why on earth would you need to clear things?!

Issue 4

No header documentation provided. All functions should include help text following Matlab's suggested format. At minimum, I would expect an H1 line, syntax description, and a description of each input and output argument (including class, size, etc.). It's very hard to test code when you have no idea what sort of inputs the functions are expecting.

somefunAgba commented 3 years ago

@kakearney thanks for the robust feedback. Issue 1 is the same with Issue 2. The problem being single quoted arguments do not allow double quoted calls in versions below R2020a. I have fixed this.

Issue 3: Thanks for that, I will comment out those housekeeping functions. They are only in a few files. Used them to keep my workspace less cluttered, kind of a working preference for me. I easily get irritated otherwise. Also all calculations are done inside defined functions with a function file. To help with testing, I am adding automated tests as @agahkarakuzu recommended.

Issue 4: I will add the minimum header to functions without one.

kakearney commented 3 years ago

After pulling the most recent version of the code and reinstalling the app, I'm still getting the same errors when trying to use the GUI. Both errors appear to be due to lack of backward compatibility.

The first error appears to originate from the save_metrics function, which specifies a 'WriteMode' parameter when it calls writetable. The 'WriteMode' option was introduced in Matlab 2020a.

The second one originates from a call to the sheetnames function; sheetnames was introduced in 2019b.

I am currently running R2019a. I do see that the README specifies that the code is only intended to be compatible back to 2019b, so the second error is my fault for overlooking that. I will try to update my version of Matlab in the next few days and retest. In the meantime, I suggest adding a version check within the code itself so that it can throw an informative error if a user tries to run it on an older version of Matlab.

if verLessThan('matlab', '9.7') % 9.7 = R2019b
    error('NLSIG-COVID19Lab requires Matlab R2019b or later');
end

As an additional note, the app appears to change the working directory when running, and when it throws an error, the user is stranded in a subfolder (in my example case, in the NLSIGCOVID19Lab/measures/infs/15-Dec-2020 folder). I recommend looking into whether the code can be reworked to avoid changing the working directory. If that cannot be avoided, it would be nice if the code took note of the original working directory and reset it when an error is caught.

majensen commented 3 years ago

I will +1 the comments of @kakearney that in general caution against the application changing any aspect of the user's environment without notification. Apply the Principle of Least Surprise and think in terms of a user who is completely ignorant and infinitely intelligent, and set that user's expectations explicitly through feedback.

somefunAgba commented 3 years ago

@kakearney I will inform you in a few days, when the app is updated. The commits done in the past few days does not reflect on the app present in the github repo. I will add the updated app on my local git repo soon.

@kakearney @majensen I will take note of that. Thanks

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

somefunAgba commented 3 years ago

@agahkarakuzu @kakearney the repo has been fully updated.

kakearney commented 3 years ago

I updated my version of Matlab to R2020b and downloaded the most recent version of the NLSIG-COVID19Lab tool, and was now able to complete my review.

To test it, I first played around with the App; I ran simulations using the default parameters and also tested entering new values in each of the dropdown menus. This new version of the app is well-designed, with a straightforward user interface and restrictions of the input that prevent the user from entering nonsense values as input... well done!

I also ran through the examples in the example folder, using the live script version, and ran the test suite. Everything appeared to run smoothly, although it did take nearly 3 hours to complete the tests. I did hit one error, described below, but I believe that is due to a typo in the test script rather than errant behavior of the code itself.

Overall, the author appears to have addressed my comments regarding respecting the user's workspace. The use of relative path names has replaced many directory changes, and with the exception of the setup script, calls to clear and clc have been removed. Thank you.

I was unable to check off items in the menu above, but have added my checklist at the bottom of this comment, with a few clarifications in italics. I believe the remaining issues are minor, and should not prevent publication of this entry.

Remaining issues

Warnings when exiting app

When closing the App, I received the following dump of warning messages. They appear harmless, but it would be nice to eliminate them if possible.

Warning: "/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/assets" not
found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/assets/01-Sep-2020"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/assets/21-Feb-2021"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/assets/22-Feb-2021"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning: "/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures/dths" not
found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures/dths/22-Feb-2021"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures/infs" not
found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures/infs/01-Sep-2020"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures/infs/21-Feb-2021"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
Warning:
"/Users/kakearney/Documents/MATLAB/Add-Ons/Apps/NLSIGCOVID19Lab/measures/infs/22-Feb-2021"
not found in path. 
> In rmpath (line 71)
In appinstall.internal.stopapp>cleanup (line 38)
In appinstall.internal.stopapp>cleanup13a (line 27)
In appinstall.internal.stopapp (line 4)
In NLSIGCOVID19LabApp>@()appinstall.internal.stopapp([],[],obj) (line 108)
In onCleanup/delete (line 80)
In closereq (line 18)
In matlab.ui.internal.controller.FigureController>@(o,e)this.Model.hgclose() (line 381)
In internal/Callback/execute (line 128)
In matlab.internal.cef/webwindow/onCustomEvent (line 1348)
In matlab.internal.cef.webwindow>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 351)
In asyncio/Channel/onCustomEvent (line 477)
In asyncio.Channel>@(source,data)obj.onCustomEvent(data.Type,data.Data) (line 409) 
>>

Test suite

I did receive one failure in the testAppUpdnModel test:

================================================================================
Verification failed in testAppUpdnModel/test_ModelButton.
    ---------------------
    Framework Diagnostic:
    ---------------------
    verifyEqual failed.
    --> The objects are not equal using "isequaln".

    Actual Value:
      datetime

       01-Sep-2020
    Expected Value:
      datetime

       01-May-2020
    ------------------
    Stack Information:
    ------------------
    In <mypath_redacted>/NLSIG-COVID19Lab/tests/testAppUpdnModel.m (testAppUpdnModel.test_ModelButton) at 65
================================================================================

The failure is the result of a possible typo in testAppUpdnModel.m, lines 64-65; the set and verify values are different, which causes that verification test to fail.

Paper

I had to look at some references to get a better understanding of what the XIR and YIR metrics meant (i.e. that they quantified inflection points in the curve). I suggest adding a line or two of text to the "NLSIG Preprint" link at the top of the readme to let readers know that that's where they should go to find the in-depth discussion of the math underlying this tool and how to best interpret results.

Also, currently, the link to the paper in the README is broken; I assume that will be fixed when the paper is finalized.

Checklist

I have placed an X at the start of each checked-off bullet, and put some comments in italics

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

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

somefunAgba commented 3 years ago

Thanks for the review @kakearney. I have fixed the remaining concerns you raised.

majensen commented 3 years ago

Hi @agahkarakuzu -- how is your review coming along? Thanks

agahkarakuzu commented 3 years ago

Hi @majensen, I am on it, I'll respond in a few days. Thank you!

agahkarakuzu commented 3 years ago

@somefunAgba I had to upgrade my MATLAB version to R2020b to meet the minimum version requirement, sorry for the delay.

I was easily able to follow the new instructions in the new README and produce expected outputs. I think users with little matlab experience would be able to do so. Thank you so much for improving the documentation and user experience.

I also see that you set up Azure pipelines for the CI tests, but they are not passing yet due to double quotes issue, for example in testScriptUpdData.m: 

status = upd_status("WD");

The problem being single quoted arguments do not allow double quoted calls in versions below R2020a. I have fixed this.

I see that your config file requests R2020b from Azure, but looks like this is still an issue. Despite that Azure installs the latest unless specified, I am suspicious that the R2020b installation did not take effect. Somewhere in your test can you ensure that the requested version matches the installed one by calling the version and maybe try:

steps:
  - task: InstallMATLAB@0
    inputs:
      release: R2020b

Another option would be simply changing double quotes with single quotes in the test script and see if the tests pass on Azure. I think it is important to have successful CI builds as well as having them set up. This is the last item on my checklist to cross off.

I am pretty much in the same boat with @kakearney regarding the scientific content of the article. Although some of the content went above my head, I was still able to understand where does this software stand in relation to the existing ones.

somefunAgba commented 3 years ago

Thanks @agahkarakuzu. I have set the release in the azure pipeline file to R2020b.

I just discovered that the reason, why the tests were failing. I also was wondering if it was not a typo issue or release issue, till I gave up.

This morning I realized that it was due to a path typo in each of the unit-test files in the test-suite folder. I have fixed this now.

Currently, the only test-suite that did not pass was testAppUpdnModel. This test-suite invokes the app, which requires that a display be present in order to test the app's graphical functionality properly. Since this cannot be currently achieved, it fails fast. The problem is described and explained in the link below: https://www.mathworks.com/matlabcentral/answers/441139-why-am-i-getting-an-error-regarding-matlab-uiautomation-driver-nodisplay-when-i-run-matlab-unit-test

In this case:

Error occurred in testAppUpdnModel/test_UpdButton and it did not run to completion.
    ---------
    Error ID:
    ---------
    'MATLAB:uiautomation:Driver:NoDisplay'
    --------------
    Error Details:
    --------------
    Error using matlab.ui.internal.Driver/press (line 8)
    Gestures on MATLAB Apps require figure display.

    Error in matlab.uitest.internal.GestureProvider/press (line 44)
                testCase.Driver.press(H, varargin{:});

    Error in testAppUpdnModel/test_UpdButton (line 44)
                testCase.press(testCase.App.UpdateDatabaseButton)

The other tests that do not require a graphics display functionality are passing.

somefunAgba commented 3 years ago

Ok. Quick update @agahkarakuzu.

All the CI tests on Azure are now passing.

I have fixed the gaphics display error by adding a test to assert whether matlab is being run in a nodisplay mode or display mode

agahkarakuzu commented 3 years ago

@somefunAgba that's great! Yes, I am aware of that display limitation, but happy that all the other issues are fixed easily!

This was the last item on my end, you may want to add badges for coverage and CI status to README if you see fit.

With these changes, I checked the last box.

majensen commented 3 years ago

@kakearney @agahkarakuzu really appreciate your careful technical reviews and @somefunAgba all the work to fix issues. I know both reviewers indicated less familiarity with the underlying mathematics; I will weigh in on this for the record (spoiler alert: looks ok and also very interesting) for the record. I will also have a readthrough of the paper itself and may make a pull request with editorial suggestions. Hope I can complete these tasks over the next week.

somefunAgba commented 3 years ago

Thanks @majensen. It has been an interesting and educating review so far for me.

majensen commented 3 years ago

@whedon check references

majensen 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 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1016/j.dib.2020.105683 is OK
- 10.1101/2020.04.13.20063354 is OK
- 10.1016/j.ejcon.2018.12.001 is OK
- 10.1007/978-0-85729-115-8_6 is OK
- 10.13140/RG.2.2.22509.95202 is OK
- 10.1101/2020.08.31.20185165 is OK
- 10.1063/1.3606555 is OK
- 10.1109/72.279181 is OK
- 10.1016/S1474-6670(17)45445-0 is OK
- 10.1016/j.ijnaoe.2018.04.002 is OK
- 10.1016/j.ejcon.2018.09.011 is OK
- 10.1109/ISCAS.2008.4541553 is OK
- 10.1109/IECON.1992.254474 is OK
- 10.1186/s12916-019-1406-6 is OK
- 10.1101/2020.07.03.20145672 is OK
- 10.1049/ic:19990713 is OK
- 10.1016/j.automatica.2006.07.018 is OK
- 10.1109/CDC.2003.1271825 is OK
- 10.1007/BF02551274 is OK
- 10.1017/atsip.2013.9 is OK
- 10.1177/1729881417703568 is OK
- 10.1007/978-3-642-36986-5_10 is OK
- 10.1038/s42256-020-00257-z is OK
- 10.1109/TNNLS.2016.2582924 is OK
- 10.1016/j.jtbi.2005.11.026 is OK
- 10.1038/35016072 is OK
- 10.1007/3-540-59497-3_175 is OK
- 10.1109/ICCV.2015.123 is OK
- 10.1162/neco.1997.9.8.1735 is OK
- 10.3201/eid1201.050396 is OK
- 10.1142/9789814261265_0009 is OK
- 10.1007/1-84628-259-4_10 is OK
- 10.1016/0005-1098(93)90052-U is OK
- 10.1007/1-84628-259-4 is OK
- 10.1016/j.conengprac.2007.03.010 is OK
- 10.1007/978-1-4842-2845-6 is OK
- 10.1109/IJCNN.2017.7966168 is OK
- 10.1007/3-540-49430-8_2 is OK
- 10.1371/journal.pone.0236860 is OK
- 10.1080/02664760903093633 is OK
- 10.1016/j.ecolmodel.2005.05.024 is OK
- 10.1063/1.4954543 is OK
- 10.1152/ajpheart.00219.2006 is OK
- 10.1109/ISTC.2014.6955084 is OK
- 10.1007/978-1-4899-7983-4 is OK
- 10.1109/JPROC.2020.2991885 is OK
- 10.3390/e21070627 is OK
- 10.3390/math8071174 is OK
- 10.1515/jee-2017-0069 is OK
- 10.1371/journal.pcbi.1002592 is OK
- 10.1016/S1474-6670(17)52049-2 is OK
- 10.1016/S1474-6670(17)58855-2 is OK
- 10.1155/2018/4231647 is OK
- 10.5772/60063 is OK
- 10.2307/2341367 is OK
- 10.1007/s13748-020-00218-y is OK
- 10.1109/ReCoSoC.2013.6581545 is OK
- 10.1016/j.neunet.2014.09.003 is OK
- 10.1073/pnas.1907373117 is OK
- 10.7641/CTA.2014.31117 is OK
- 10.7763/IJCTE.2009.V1.13 is OK
- 10.1016/j.ijid.2020.04.085 is OK
- 10.1080/10511979808965879 is OK
- 10.1049/cp:19991129 is OK
- 10.1109/JPROC.2017.2761740 is OK
- 10.1186/1742-4682-2-14 is OK
- 10.1007/s10462-011-9294-y is OK
- 10.1080/00031305.2017.1380080 is OK
- 10.1016/j.spl.2011.11.012 is OK
- 10.1109/5.58337 is OK
- 10.1007/s11071-020-05862-6 is OK
- 10.1109/IJCNN.2018.8489043 is OK
- 10.1016/j.jtbi.2012.07.024 is OK
- 10.1109/APEC.2016.7468308 is OK
- 10.1093/aob/mcg029 is OK
- 10.1109/TVLSI.2012.2232321 is OK

MISSING DOIs

- 10.1016/0005-1098(86)90095-6 may be a valid DOI for title: Computer-Controlled Systems: Theory and Design
- 10.1016/0005-1098(93)90084-7 may be a valid DOI for title: Linear Controller Design: Limits of Performance
- 10.1561/9781680833294 may be a valid DOI for title: Multi-Period Trading via Convex Optimization
- 10.1201/9781315219165 may be a valid DOI for title: Feedback, Nonlinear, and Distributed Circuits
- 10.9790/4200-0702012633 may be a valid DOI for title: Performance Analysis of the Sigmoid and Fibonacci Activation Functions in NGA Architecture for a Generalized Independent Component Analysis
- 10.1007/978-1-4020-6949-9 may be a valid DOI for title: Handbook of Continued Fractions for Special Functions
- 10.1007/978-3-319-09330-7_7 may be a valid DOI for title: Training Deep Fourier Neural Networks To Fit Time-Series Data
- 10.1109/cvpr.2016.90 may be a valid DOI for title: Deep Residual Learning for Image Recognition
- 10.1109/ism.2016.0052 may be a valid DOI for title: Fair and Efficient Bandwidth Allocation for Video Flows Using Sigmoidal Programming
- 10.3390/info6030432 may be a valid DOI for title: Sliding-Mode Speed Control of PMSM with Fuzzy-Logic Chattering Minimization—Design and Implementation
- 10.23943/princeton/9780691197296.001.0001 may be a valid DOI for title: Statistical Inference via Convex Optimization
- 10.21468/scipostphys.9.4.053 may be a valid DOI for title: Neural Network-Based Approach to Phase Space Integration
- 10.1109/tnnls.2013.2281217 may be a valid DOI for title: Local Stability Analysis of Discrete-Time, Continuous-State, Complex-Valued Recurrent Neural Networks with Inner State Feedback
- 10.3390/books978-3-03897-522-9 may be a valid DOI for title: Biological and Biogenic Crystallization
- 10.1613/jair.251 may be a valid DOI for title: Mean Field Theory for Sigmoid Belief Networks
- 10.1007/978-981-13-1298-4_1 may be a valid DOI for title: Intelligent Adaptive Fuzzy Control
- 10.1109/icdar.2003.1227801 may be a valid DOI for title: Best Practices for Convolutional Neural Networks Applied to Visual Document Analysis
- 10.1016/j.physa.2010.01.035 may be a valid DOI for title: Improvement of Signal-to-Noise Ratio by Stochastic Resonance in Sigmoid Function Threshold Systems, Demonstrated Using a CMOS Inverter
- 10.46945/bpj.10.1.03.01 may be a valid DOI for title: WHO Coronavirus Disease (COVID-19) Dashboard

INVALID DOIs

- None
majensen commented 3 years ago

@somefunAgba - there are number of references that Whedon found DOIs for (see above).

I think the easiest thing is to edit the paper.bib so that it includes only the references you need in the paper.

thanks

majensen commented 3 years ago

@somefunAgba, here is my understanding and overall impression of the method itself. Welcome your comments.

My impression of the story.

Logistic growth is characteristic of many, many natural phenomena. It captures key real world system behaviors of a maximal growth rate and also of diminishing returns. As a growth system's external influences change, logistic growth should remain a good representation of the new system behavior, provided that the growth parameters are adjusted to the changing influences. In fact, one can imagine adding together (superposing) multiple logistic functions of appropriate parameters, appropriately translated along the domain axis, to estimate or fit any time series whose underlying process is one of growth constrained by limited resources.

Fitting an appropriate superposition of logistic functions to data is based on a model of the underlying process, in contrast to other forms of estimation (such as stitching together splines at their endpoints), whose goal is simply to minimize the difference between data points and an arbitrary smooth function. The authors make a key insight, realizing that superposition of identically-parametered logistic sigmoid functions is equivalent to a certain neural network, the activation function of whose nodes are precisely the functions to be fitted. This, and the fact that the functions are of identical form, enables the authors to bring to bear numerical neural network theory on the paramter fitting problem - which becomes an optimization problem for which, in these days of deep learning, has many efficient tools.

Another key insight of the method is the reduction of dimensionality, by fitting, not all data points, but only points of sigmoid inflection. This takes advantage of the assumption of the underlying process (logistic growth) driving the dynamics, and presumably leads to a better foundation for a mechanistic interpretation of the fit.

The authors are able to make use of a variant of the Kolmogorov-Smirnov inequality to derive meaningful assessments of goodness of fit and confidence intervals on the point estimations of the time series. In my opinion, the goodness-of-fit results, at least in the examples provided in the paper, are a vindication of the method. But the logic of the derivation is also clear and seems sound to me.

majensen commented 3 years ago

I have made https://github.com/somefunAgba/NLSIG-COVID19Lab/pull/4 with some very minor edits for consideration.

somefunAgba commented 3 years ago

Thank you @majensen. Your review of the paper is accurate.

somefunAgba commented 3 years ago

I have merged the pull requests

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

majensen commented 3 years ago

Ok @somefunAgba - the last thing to do before we request publication is to create an open archive of the software. This can be done using Zenodo, FigShare, or similar service. There is a short tutorial at this reference. Once you create the archive, can you please report the version number and the archive DOI back in this thread? Thank you.

somefunAgba commented 3 years ago

@majensen, using zenodo, here are the version number and archive number Version 2.1.1 https://doi.org/10.5281/zenodo.4662275

majensen commented 3 years ago

Thanks @somefunAgba - if you can, please go ahead and update the metadata at the archive to remove my name from the author list (that's just a Github automatic population), and then add your co-authors names.

somefunAgba commented 3 years ago

@majensen Done

agahkarakuzu commented 3 years ago

@majensen that was such a helpful comment! Despite that logistic growth is not my bread and butter, your comment gave me a great starting point to see what is really cool about considering multiple inflection points sampled at irregular finite intervals when modeling the limits of the exponential growth. I gave Somefun et al. 2020 another read with the encouragement from your high-level insight, then it all clicked into place, including the significance of using cumulative growth.

Looking forward to the time point when we are at the phase of perceiving covid-19 as a past finite incident, and remember how NLSIG provided an efficient solution to capture the true nature of its bounds, even while it was still unbearably continuous.

Thank you @somefunAgba for your contribution and careful consideration of the issues we raised about the codebase quality/ux with @kakearney, @majensen for the insightful summary of the methodology and JOSS for making all these conversations publicly available. This review proved me once again that reinforcing understanding by reading comments is a unique learning experience.

majensen commented 3 years ago

@whedon set 2.1.1 as version

whedon commented 3 years ago

OK. 2.1.1 is the version.

majensen commented 3 years ago

@whedon set 10.5281/zenodo.4662275 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4662275 is the archive.

majensen commented 3 years ago

@whedon accept