Closed whedon closed 4 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @Shibabrat 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
Reference check summary:
OK DOIs
- None
MISSING DOIs
- https://doi.org/10.1016/0005-1098(89)90019-8 may be missing for title: System identification: theory for the user
- https://doi.org/10.1080/00207178908559683 may be missing for title: Representations of non-linear systems: the NARMAX model
- https://doi.org/10.1016/j.ymssp.2015.12.031 may be missing for title: Sufficient conditions for rate-independent hysteresis in autoregressive identified models
- https://doi.org/10.1109/access.2019.2926057 may be missing for title: Control of Hysteretic Systems Through an Analytical Inverse Compensation Based on a NARX Model
- https://doi.org/10.1016/s0307-904x(03)00071-4 may be missing for title: Modelling and prediction of machining errors using ARMAX and NARMAX structures
- https://doi.org/10.1016/j.neucom.2015.08.022 may be missing for title: Ultra-orthogonal forward regression algorithms for the identification of non-linear dynamic systems
- https://doi.org/10.1109/tbme.2002.803507 may be missing for title: NARMAX representation and identification of ankle dynamics
- https://doi.org/10.1016/b978-0-12-811788-0.00008-1 may be missing for title: Applications of NARMAX in Space Weather
INVALID DOIs
- None
@whedon add @dawbarton as reviewer
OK, @dawbarton is now a reviewer
👋 @arfon, I seem to have messed up the reviewer assignment, sorry. @dawbarton doesn't seem to appear as a reviewer. Should I fix this by hand?
👋 @arfon, I seem to have messed up the reviewer assignment, sorry. @dawbarton doesn't seem to appear as a reviewer. Should I fix this by hand?
@dpsanders - Yes, I'm afraid Whedon doesn't yet know how to add a checklist for a reviewer once the review has started.
You can do this by editing the body of the issue at the top of this thread. Let me know if you need my help with this.
@arfon OK thanks, done.
Overall, looks good. It installs easily and usage is as described. Documentation is good overall.
A few detailed comments below.
introduction_to_narmax.html
: $F^l$ in the "So, what is a NARMAX model?" section appears to be slightly misleading. The notation seems to indicate a polynomial function (particularly with the comment about setting $l=1$ recovering the ARMAX model) whereas you describe it as a general nonlinear function. In the section that follows, you refer to "unknown mapping $f[â‹…]$"; I presume this was the $F^l$ in the previous section.user_guide.html
: the formatting of the text goes awry about halfway down (all the text appears to turn into headings). The text is fine, though you might want to consider how it is structured. I would recommend putting the function you are describing first and then explain it, rather than give a (rather specific) explanation and then the function. For example, the get_siso_data
function - you start by giving an equation then say that color_noise
equal to True
means that you get a certain noise relationship. In the context of the equation (alone) that statement is confusing - what has color_noise
got to do with the equation? It doesn't have any variables called color_noise
in it. It's not until the end of the section that you see that the text was actually talking about a particular function (that is, get_siso_data
) to generate sample data.Dear @dawbarton , we appreciate the time and effort that you dedicated to providing feedback on our manuscript.
Paper - some comments scribbled onto the PDF 10.21105.joss.02384 1.pdf; it's a bit sloppy in places and looks like it really needed proof reading again. Regarding the manuscript, we are incorporating the suggestions made by you. The comments will result in valuable improvements to our paper.
introduction_to_narmax.html: $F^l$ in the "So, what is a NARMAX model?" section appears to be slightly misleading. The notation seems to indicate a polynomial function (particularly with the comment about setting $l=1$ recovering the ARMAX model) whereas you describe it as a general nonlinear function. In the section that follows, you refer to "unknown mapping $f[â‹…]$"; I presume this was the $F^l$ in the previous section. Thank you for pointing this out. The $F$ in NARMAX indicates a general nonlinear function (Neural, Fuzzy, and many other). However, because we define it as a polynomial function, we have to keep the definition consistent. I will upload the changes soon.
user_guide.html: the formatting of the text goes awry about halfway down (all the text appears to turn into headings). The text is fine, though you might want to consider how it is structured. We agree with you. The documentation is in a very early stage. Actually, the website version will be replaced very soon. Many issues with structure and formatting is due the current way it is done. We are converting the docs to a properly website.
In this case, I have a question: Would these changes concerning the documentation in the website be mandatory in this revision process?
I'm not sure what the Jupyter notebooks section of the docs is supposed to do - it appears to be largely the same as the user guide. Each notebook presents a different exemple of how particular parameters works and some tips. We made this section for the users who have no prior knowledge of the library or NARMAX models. There are particular usage of some parameters we didn't show in user guide.
I can't see any automated tests in the repo - have I missed them or do they not exist? I think JOSS requires them? We have tests that coverage approximately 87% of the our functions. These tests are placed on "tests" folder inside each class path.
@whedon generate pdf
@whedon generate pdf
Dear @dawbarton, the paper was updated following your suggestions. if you want to add any comments, just let me know.
Regarding my previous comment regarding the documentation, please let me know if the those suggestions would be mandatory. I am working on moving the documentation to a tool other than Sphinx. However, if corrections to the web documentation are necessary I will update the current files.
All looks good to me now. I've ticked off all the items on the checklist.
Thanks a lot @dawbarton!
@Shibabrat Just checking in to see how progress is with your review?
@wilsonrljr: I was wondering if http://www.nonlinearbenchmark.org/ is relevant?
Thank you, @dawbarton!
@dpsanders Regarding the that site, it is really good! It contains a set of really great data for system identification. I have an exemple here for the F-16 dataset (since I have used it in my dissertation). I'll add an notebook in example folder later today. I have other examples with real world data used as benchmark for SI already implemented and I'll upload them in the next update of the package. However, I'll add the F-16 example later today because those data are really good.
Regarding http://www.nonlinearbenchmark.org/ I should mention that it's quite a nice meeting that runs each year and is directly relevant to your software package; it's well worth attending if the situation ever allows. (I went a few years ago.)
Thanks for pointing that out, dawbarton. I worked with some of those datasets on my master dissertation. Some of the authors collaborating with the website are reference on system identification using NARMAX models. It's really important to share our work with researchers on the field with examples that make sense for them.
I updated the master branch with the F-16 example. It's a simple showcase for now, without any comparison with other works. https://wilsonrljr.github.io/sysidentpy/examples/f_16_benchmark.html
👋 @Shibabrat: Just checking in to see if you have been able to make progress with your review on this submission? Thanks!
👋 @Shibabrat: Just checking in to see if you have been able to make progress with your review on this submission? Thanks!
Hi @dpsanders, I have a few more things to check and will be finishing them by 10th Sept.
@dawbarton I've finished my review and satisfied with the current version of the package.
@wilsonrljr can you add a section on testing to the README? I see the tests are available but it may not be obvious to a first time user. Otherwise, LGTM.
Thanks, @Shibabrat!
Following your suggestion, I added the following note to the README on Development section:
"Note: we use the pytest package for testing. The test functions are located in tests subdirectories at each folder inside sysidentpy, which check the validity of the algorithms.
Run the pytest in the respective folder to perform all the tests of the corresponding sub-packages.
Currently, we have around 85% of code coverage."
I wouldn't know how to "run the pytest". Could you make that more explicit, please?
Absolutely! Later today I will add an example of how to do that.
I added more information concerning the tests, including an example of how to run it. If you have any more suggestions, just let me know!
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Once the final details have been fixed, I think we're ready to proceed to publication!
Could you please make a tagged release and archive the software (e.g. on Zenodo or Figshare), and report the version number and archive DOI here. Thanks!
I made a new release (release name -> SysIdentPy v0.1.2, version v0.1.2).
Zenodo registered two DOIs:
@wilsonrljr The author list on Zenodo does not match the list on the paper.
Sorry about that, dpsanders. Zenodo generated it automatically based on the last contributors/commits. It's fixed now.
Thanks! and sorry for the delay here.
I'm going to proceed towards publication!
@whedon set 10.5281/zenodo.4026516 as archive
OK. 10.5281/zenodo.4026516 is the archive.
@whedon set 0.1.2. as version
OK. 0.1.2. is the version.
@whedon set 0.1.2 as version
OK. 0.1.2 is the version.
@whedon accept
Attempting dry run of processing paper acceptance...
:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.
Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/1763
If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1763, then you can now move forward with accepting the submission by compiling again with the flag deposit=true
e.g.
@whedon accept deposit=true
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- None
MISSING DOIs
- 10.1016/0005-1098(89)90019-8 may be a valid DOI for title: System identification: theory for the user
- 10.1080/00207178908559683 may be a valid DOI for title: Representations of non-linear systems: the NARMAX model
- 10.1016/j.ymssp.2015.12.031 may be a valid DOI for title: Sufficient conditions for rate-independent hysteresis in autoregressive identified models
- 10.1109/access.2019.2926057 may be a valid DOI for title: Control of Hysteretic Systems Through an Analytical Inverse Compensation Based on a NARX Model
- 10.1016/s0307-904x(03)00071-4 may be a valid DOI for title: Modelling and prediction of machining errors using ARMAX and NARMAX structures
- 10.1016/j.neucom.2015.08.022 may be a valid DOI for title: Ultra-orthogonal forward regression algorithms for the identification of non-linear dynamic systems
- 10.1109/tbme.2002.803507 may be a valid DOI for title: NARMAX representation and identification of ankle dynamics
- 10.1016/b978-0-12-811788-0.00008-1 may be a valid DOI for title: Applications of NARMAX in Space Weather
- 10.1016/j.automatica.2020.109099 may be a valid DOI for title: A Tree Adjoining Grammar Representation for Models Of Stochastic Dynamical Systems
INVALID DOIs
- None
@wilsonrljr Could you please fix the missing DOIs identified above? Thanks!
@dpsanders I've fixed the missing DOIs.
@whedon accept
Attempting dry run of processing paper acceptance...
:wave: @openjournals/joss-eics, this paper is ready to be accepted and published.
Check final proof :point_right: https://github.com/openjournals/joss-papers/pull/1765
If the paper PDF and Crossref deposit XML look good in https://github.com/openjournals/joss-papers/pull/1765, then you can now move forward with accepting the submission by compiling again with the flag deposit=true
e.g.
@whedon accept deposit=true
Submitting author: @wilsonrljr (Wilson Rocha Lacerda Junior) Repository: https://github.com/wilsonrljr/sysidentpy Version: 0.1.2 Editor: @dpsanders Reviewers: @Shibabrat, @dawbarton Archive: 10.5281/zenodo.4026516
: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
@Shibabrat and @dawbarton, 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 @dpsanders know.
✨ Please try and complete your review in the next six weeks ✨
Review checklist for @Shibabrat
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
[x] References: Is the list of references complete, and is everything cited appropriately that should be cited (e.g., papers, datasets, software)? Do references in the text use the proper citation syntax?
Review checklist for @dawbarton
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper