Closed whedon closed 3 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @moorepants, @dineshresearch 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.1163/1574-9347_dnp_e612900 may be missing for title: Keras
INVALID DOIs
- None
Thanks for the comments from @madcowswe in the pre-rewie thread #2275. Please find some answers below:
1. Figure1: The convention I am used to (passive sign convention), the arrows for Usup and Uin should point from bottom to up (the other way).
The sign convention is an arbitrary choice. In energy conversion engineering (such as power electronic and drive systems) the so-called generator convention is mostly used in the international literature where the voltage arrow is pointing down as indicated.
2. Figrue1: The omega_me direction is ambigous as it's annotated to the rotation arrows in both directions
If you may have a closer look on the arrow labeling you can inspect that on the motor side we indicate both the rotation and torque orientation while on the mechanical load side we point out that the load torque has an opposite orientation compared to the motor torque (as usual in any free body diagram) and that the load torque is or might be depending on the shaft speed T(omega). Hence, on the shaft there are in total two torques indicated while the mechanical rotation orientation is only labeled once.
3. Figure 2: It seems that the dq frame presented for the bottom right plot (the current) is the rotor frame. However, the dq frame I am expecting for an asynchronous motor is one where the d axis is aligned to the rotor flux.
Sure, the usual field-oriented motor control definition is that the dq coordinate system is aligned towards the rotor flux (induction or synchronous machines). And this coordinate transformation can be directly used to also transform other important electrical motor quantities like the phase current or phase voltage into that very same dq coordinate framework. Based on that idea the usual field-oriented control starts with a current control loop using the dq-transformed currents. If you are interesting in more details on that I can recommend these lecture notes or the book by W. Leonhard 'Control of Electrical Drives' ,Springer, 2001.
I'm in the middle of an across the world move, so I probably won't get to this for two or more weeks.
@moorepants okay thanks for letting us know
Here is my first pass on a review:
Contribution and authorship: Has the submitting author (@wkirgsn) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?
Some of the paper authors, including the first author, do not seem to have made any commits to the software repository. Please clarify the authorship of all the authors.
These sentences "In particular, the domain of three-phase drives makes use of coordinate transformations to view these ODEs in a more interpretable frame. In GEM, both, the physical (abc-) and the simplified (dq-)coordinates are available to be used as the frame of input and output variables, allowing for easy and quick controller analysis and diagnose within the most convenient coordinate frame" are not understandable without some domain knowledge. Please define "frame" in this context as well as "physical (abc-) and the simplified (dq-)".
I don't think "generatoric" is a word in the English language.
You list 4 examples in the paper but these are not present in the documentation. I suggest adding these examples to the documentation so that users can easily find and understand the examples. The examples, as written are not easily interpretable by a user. There are very few comments in the files explaining what is going on. I think you need a tutorial style section of the documentation to teach users how to use your software. I don't think I could figure it out with the present information.
Functionality: Have the functional claims of the software been confirmed?
This is not apparent from the documentation or the paper. You show one plot in the paper that is created from the software, but there is no associated code that I can run to reproduce that plot. I think a tutorial explaining your four examples that provide the inputs the user types and the subsequent outputs that the software produces would demonstrate the functionality (at least for those for examples).
Installation
My attempt at installing the software failed. Here is the session:
moorepants@garuda:~$ conda create -n gem python=3
Collecting package metadata (current_repodata.json): done
Solving environment: done
## Package Plan ##
environment location: /home/moorepants/miniconda3/envs/gem
added / updated specs:
- python=3
The following packages will be downloaded:
package | build
---------------------------|-----------------
_libgcc_mutex-0.1 | conda_forge 3 KB conda-forge
_openmp_mutex-4.5 | 1_gnu 22 KB conda-forge
certifi-2020.6.20 | py38h32f6830_0 151 KB conda-forge
python-3.8.5 |h4d41432_2_cpython 71.0 MB conda-forge
python_abi-3.8 | 1_cp38 4 KB conda-forge
setuptools-49.2.1 | py38h32f6830_0 931 KB conda-forge
------------------------------------------------------------
Total: 72.1 MB
The following NEW packages will be INSTALLED:
_libgcc_mutex conda-forge/linux-64::_libgcc_mutex-0.1-conda_forge
_openmp_mutex conda-forge/linux-64::_openmp_mutex-4.5-1_gnu
ca-certificates conda-forge/linux-64::ca-certificates-2020.6.20-hecda079_0
certifi conda-forge/linux-64::certifi-2020.6.20-py38h32f6830_0
ld_impl_linux-64 conda-forge/linux-64::ld_impl_linux-64-2.34-hc38a660_9
libffi conda-forge/linux-64::libffi-3.2.1-he1b5a44_1007
libgcc-ng conda-forge/linux-64::libgcc-ng-9.3.0-h24d8f2e_14
libgomp conda-forge/linux-64::libgomp-9.3.0-h24d8f2e_14
libstdcxx-ng conda-forge/linux-64::libstdcxx-ng-9.3.0-hdf63c60_14
ncurses conda-forge/linux-64::ncurses-6.2-he1b5a44_1
openssl conda-forge/linux-64::openssl-1.1.1g-h516909a_1
pip conda-forge/noarch::pip-20.2-py_0
python conda-forge/linux-64::python-3.8.5-h4d41432_2_cpython
python_abi conda-forge/linux-64::python_abi-3.8-1_cp38
readline conda-forge/linux-64::readline-8.0-he28a2e2_2
setuptools conda-forge/linux-64::setuptools-49.2.1-py38h32f6830_0
sqlite conda-forge/linux-64::sqlite-3.32.3-hcee41ef_1
tk conda-forge/linux-64::tk-8.6.10-hed695b0_0
wheel conda-forge/noarch::wheel-0.34.2-py_1
xz conda-forge/linux-64::xz-5.2.5-h516909a_1
zlib conda-forge/linux-64::zlib-1.2.11-h516909a_1006
Downloading and Extracting Packages
_openmp_mutex-4.5 | 22 KB | ############################################################## | 100%
python-3.8.5 | 71.0 MB | ############################################################## | 100%
_libgcc_mutex-0.1 | 3 KB | ############################################################## | 100%
setuptools-49.2.1 | 931 KB | ############################################################## | 100%
certifi-2020.6.20 | 151 KB | ############################################################## | 100%
python_abi-3.8 | 4 KB | ############################################################## | 100%
Preparing transaction: done
Verifying transaction: done
Executing transaction: done
#
# To activate this environment, use
#
# $ conda activate gem
#
# To deactivate an active environment, use
#
# $ conda deactivate
moorepants@garuda:~$ act gem
(gem) moorepants@garuda:~$ which pip
/home/moorepants/miniconda3/envs/gem/bin/pip
(gem) moorepants@garuda:~$ pip install gym-electric-motor
Collecting gym-electric-motor
Downloading gym_electric_motor-0.2.1-py3-none-any.whl (140 kB)
|████████████████████████████████| 140 kB 1.5 MB/s
Collecting scipy>=1.4.1
Downloading scipy-1.5.2-cp38-cp38-manylinux1_x86_64.whl (25.7 MB)
|████████████████████████████████| 25.7 MB 3.0 MB/s
Collecting pytest-cov
Downloading pytest_cov-2.10.0-py2.py3-none-any.whl (19 kB)
Collecting PyQT5==5.9.2
Downloading PyQt5-5.9.2-5.9.3-cp35.cp36.cp37-abi3-manylinux1_x86_64.whl (105.3 MB)
|████████████████████████████████| 105.3 MB 26 kB/s
Collecting matplotlib>=3.1.0
Downloading matplotlib-3.3.0-1-cp38-cp38-manylinux1_x86_64.whl (11.5 MB)
|████████████████████████████████| 11.5 MB 2.5 MB/s
Collecting numpy>=1.16.4
Downloading numpy-1.19.1-cp38-cp38-manylinux2010_x86_64.whl (14.5 MB)
|████████████████████████████████| 14.5 MB 655 kB/s
Collecting pytest>=5.2.2
Downloading pytest-6.0.1-py3-none-any.whl (270 kB)
|████████████████████████████████| 270 kB 1.6 MB/s
Collecting gym>=0.15.4
Downloading gym-0.17.2.tar.gz (1.6 MB)
|████████████████████████████████| 1.6 MB 2.3 MB/s
Collecting coverage>=4.4
Downloading coverage-5.2.1-cp38-cp38-manylinux1_x86_64.whl (231 kB)
|████████████████████████████████| 231 kB 2.5 MB/s
ERROR: Could not find a version that satisfies the requirement sip<4.20,>=4.19.4 (from PyQT5==5.9.2->gym-electric-motor) (from versions: 5.0.0, 5.0.1, 5.1.0, 5.1.1, 5.1.2, 5.2.0, 5.3.0)
ERROR: No matching distribution found for sip<4.20,>=4.19.4 (from PyQT5==5.9.2->gym-electric-motor)
Also not clear why testing related packages are installed as dependencies, shouldn't these only be required if you are developing the package?
Statement of need
This could be more explicit and there could be more. When I read the paper I interpreted it as Matlab does this already but doesn't connect to keras/tensorflow well, so you need this package. That is a sufficient need, but is that the main/only need?
Installation instructions
I don't see an explicit list of dependencies. This would be helpful for setting up a conda environment, for example. There is also a dependency issue pip encounters as shown above. I don't see any info in the docs about dependencies that would help me debug this.
Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
There are 4 examples but they need to be improved to be understandable by a new user.
Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
API docs look good.
Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified?
Looks like there are tests, but haven't been able to run due to installation issue.
Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided?
Needs some improvement for non-specialist audience. Define or reduce jargon, for example.
State of the field: Do the authors describe how this software compares to other commonly-used packages?
Little is explained about this besides saying that Matlab does something similar. More detail is needed here.
Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
There are some improvements in grammar needed.
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?
This seems brief. Maybe you'll have more references to actual electric motor software once you improve your state of the field section.
Thank you for the very constructive review, @moorepants! We appreciate every remark and are eager to improve on these discrepancies.
We are working on a major update of the repo, which becomes manifest in the nightly
branch. There, the dependency problem preventing you from installation is already solved, along the incorporation of more features and bug fixes.
Although this development branch is already quite packed and far ahead of the master
, we're working on completing tutorials that were also requested by your review, that will delay the eventual merge of the nightly
into master
until presumably mid September.
For the meantime, please use GEM master
with python 3.6 or 3.7, or just the nightly
branch. I am going to add a restriction on the python version for the master branch for the time being until the major update to prevent other users from experiencing these issues.
@wkirgsn any updates to report?
@dineshresearch when can we expect your review contribution? Thanks again for your help
@Kevin-Mattheus-Moerman The initial estimate of mid september for the major update still seems to hold for now. The nightly branch is updated continuously, and the paper is also currently worked on, incorporating moorepants' remarks.
As @wkirgsn announced we have revised the paper based on the reviewer remarks. Hence, we are looking forward to further comments on the paper.
In the meantime we are working on a major update of the master branch, which can already be previewed on the nightly branch.
@whedon generate pdf
@dineshresearch are you still able to help with this review? Please let us know. Thanks.
Hello,
Sorry for the late reply because of my busy schedule. Invitation has expired for me and can you please send me a new invite.
On Wed, Aug 26, 2020 at 2:50 AM Kevin Mattheus Moerman < notifications@github.com> wrote:
@dineshresearch https://github.com/dineshresearch are you still able to help with this review? Please let us know. Thanks.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2498#issuecomment-680275659, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGIAFMRRV27WRODZE7WL4KDSCQTILANCNFSM4PENCHZA .
-- Kind Regards Amara Dinesh Kumar “Alone we can do so little and together we can do much” — Helen Keller Research Info https://sites.google.com/view/dineshkumaramara/home | Github Profile https://github.com/dineshresearch | Cybersecurity-Lab@CEN https://vinayakumarr.github.io/Cybersecurity-Lab-at-CEN/ | ComputationalThinking@CEN https://vinayakumarr.github.io/ComputationalThinking-Lab-at-CEN/ | ControlsLab http://cen-controls-lab.github.io/
@whedon re-invite @dineshresearch as reviewer
OK, the reviewer has been re-invited.
@dineshresearch please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations
Hello, Sorry for the late reply because of my busy schedule. Invitation has expired for me and can you please send me a new invite.
Thanks @dineshresearch for getting back to us. Let me know if you have any questions/difficulties.
@dineshresearch :wave: were you able to accept that invitation to get started? Thanks
@dineshresearch please can you finalize this review at your earliest convenience? Thanks again for your help.
@dineshresearch :wave:
Sorry for the delay as I was busy but will submit the review in next 2 days
On Thu, Sep 10, 2020 at 5:41 PM Kevin Mattheus Moerman < notifications@github.com> wrote:
@dineshresearch https://github.com/dineshresearch 👋
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2498#issuecomment-690233263, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGIAFMRCVOHXEA4SIZLYJ5LSFC665ANCNFSM4PENCHZA .
-- Kind Regards Amara Dinesh Kumar “Alone we can do so little and together we can do much” — Helen Keller Research Info https://sites.google.com/view/dineshkumaramara/home | Github Profile https://github.com/dineshresearch | Cybersecurity-Lab@CEN https://vinayakumarr.github.io/Cybersecurity-Lab-at-CEN/ | ComputationalThinking@CEN https://vinayakumarr.github.io/ComputationalThinking-Lab-at-CEN/ | ControlsLab http://cen-controls-lab.github.io/
@dineshresearch please finalize the review at your earliest convenience. If you are no longer able to complete this review please let us know so we can find an alternative reviewer. Thanks for your help.
@dineshresearch :wave: please finish your review so we do not delay this submission any further. Thanks
Hello I have completed the review checklist..
@Kevin-Mattheus-Moerman I'm pretty sure I didn't check all of the boxes in the review check list under my name, as I had comments regarding several of them. I'm not sure why they are all checked.
@Kevin-Mattheus-Moerman I'm pretty sure I didn't check all of the boxes in the review check list under my name, as I had comments regarding several of them. I'm not sure why they are all checked.
@moorepants sorry about that. Not sure perhaps @dineshresearch ticked some by accident? Feel free to untick the boxes you meant not to tick yet.
@dineshresearch thanks for your help here! I see you have left the State of the field
box :point_up: unchecked. Can you summarize for the authors what action is needed here?
The toolbox just experienced its major update by merging nightly into master. We now stand on v0.3.0, with plenty of new additions, bug fixes and comprehensive tutorials as well as example scripts. These include the feedback of our reviewers, and we hope that discrepancies are decimated now.
We are looking forward to any feedback and appreciate your time.
@wkirgsn Thank you. It would be helpful if you address my comments explicitly and point to specific new content that address each.
Hi @moorepants, please see the detailed response below:
"Some of the paper authors, including the first author, do not seem to have made any commits to the software repository. Please clarify the authorship of all the authors." Email addresses were updated on github among authors, such that code contributions should be addressable now. Github Insights might not show the updated data right off, but the box "contributors" on the repo frontpage includes all authors now, and are reflected in the git history in any case.
These sentences "In particular, the domain of three-phase drives makes use of coordinate transformations to view these ODEs in a more interpretable frame. In GEM, both, the physical (abc-) and the simplified (dq-)coordinates are available to be used as the frame of input and output variables, allowing for easy and quick controller analysis and diagnose within the most convenient coordinate frame" are not understandable without some domain knowledge. Please define "frame" in this context as well as "physical (abc-) and the simplified (dq-)".
Sentence was reworked in the revised JOSS paper.
"I don't think "generatoric" is a word in the English language." Sentence was reworked in the revised JOSS paper.
"You list 4 examples in the paper but these are not present in the documentation. I suggest adding these examples to the documentation so that users can easily find and understand the examples. The examples, as written are not easily interpretable by a user. There are very few comments in the files explaining what is going on. I think you need a tutorial style section of the documentation to teach users how to use your software. I don't think I could figure it out with the present information." The example scripts are reworked completely with added documentation. Moreover, several tutorial notebooks were introduced (links in README) for a step-by-step guide to various aspects of the toolbox
"Functionality: Have the functional claims of the software been confirmed? This is not apparent from the documentation or the paper. You show one plot in the paper that is created from the software, but there is no associated code that I can run to reproduce that plot. I think a tutorial explaining your four examples that provide the inputs the user types and the subsequent outputs that the software produces would demonstrate the functionality (at least for those for examples)." Several tutorial notebooks were introduced (links in README) for a step-by-step guide to various aspects of the toolbox
"Installation My attempt at installing the software failed. Here is the session: .." Any installation related problems were rectified. In particular, Python 3.8 is supported now.
"Also not clear why testing related packages are installed as dependencies, shouldn't these only be required if you are developing the package?" We feel that also mere users of the toolbox might want to ensure that what they use is also thoroughly tested. The pytest packages are rather lightweight and shouldn't be perceived as a burden.
"Statement of need This could be more explicit and there could be more. When I read the paper I interpreted it as Matlab does this already but doesn't connect to keras/tensorflow well, so you need this package. That is a sufficient need, but is that the main/only need?" This was more elaborated on in the revised JOSS paper.
"Installation instructions
I don't see an explicit list of dependencies. This would be helpful for setting up a conda environment, for example. There is also a dependency issue pip encounters as shown above. I don't see any info in the docs about dependencies that would help me debug this."
Any required packages are compiled in the requirements.txt
as is usual for python packages. They'll be automatically installed when executing setup.py, or alternatively when installing gem with pip.
"Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems). There are 4 examples but they need to be improved to be understandable by a new user." The example scripts are reworked completely with added documentation. Moreover, several tutorial notebooks were introduced (links in README) for a step-by-step guide to various aspects of the toolbox
"Automated tests: Are there automated tests or manual steps described so that the functionality of the software can be verified? Looks like there are tests, but haven't been able to run due to installation issue." All installation issues should be resolved now. Alternatively, Travis-CI builds the repo by running all tests with coverage analysis.
"Summary: Has a clear description of the high-level functionality and purpose of the software for a diverse, non-specialist audience been provided? Needs some improvement for non-specialist audience. Define or reduce jargon, for example. State of the field: Do the authors describe how this software compares to other commonly-used packages? Little is explained about this besides saying that Matlab does something similar. More detail is needed here. Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)? There are some improvements in grammar needed. 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? This seems brief. Maybe you'll have more references to actual electric motor software once you improve your state of the field section."
This was more elaborated on in the revised JOSS paper.
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi @Kevin-Mattheus-Moerman, we revised the toolbox and the JOSS paper candidate according to the reviewers' remarks. Can you please advice what the next steps are, or which obstacles need to be further overcome in order to proceed to an acceptance and publication? We appreciate your and the reviewers' time and effort as always.
@wkirgsn apologies for the delay. It looks like the reviewers have to pick this up again. I will remind them now.
I've got it on my TODO list. Hoping this week.
@moorepants, @dineshresearch the authors have made a number of changes which are ready for you to review.
@moorepants perhaps the other reviewer ticked all your boxes by mistake. Could you untick the boxes you feel should not be ticked yet? Thanks.
@dineshresearch you have 1 box unticked, "state of the field", can you elaborate on this to the authors so they know what to work on if this is still needed?
If either of you are happy for this work to be published please let me know.
Thanks!
@dineshresearch @moorepants, this is a gentle reminder about the above :point_up:, thanks
@dineshresearch @moorepants, are you able to pick this up? Thanks
I have given the repo and paper a second pass. There has been significant improvements. All of my comments have been addressed and the package is looking really nice now. Congrats on the great work!
I am having trouble getting the Colab notebooks to run and they give output:
Last cell gives me:
give me:
gives:
So there are some new errors.
I also still am having trouble installing locally (Ubuntu 20.04):
Thanks @moorepants !!
@wkirgsn can you look at/address these issues :point_up:
Thank you @moorepants for your second review! You raised very good points of which we weren't aware of. After analyzing the problems we found the root cause of them all to be rather peripheral.
So all those points are either fixed or going to fix themselves in the medium run. Thank you for your time again, and we appreciate all feedback given so far.
Ok, I installed the package successfully with Python 3.8 and tested an example.
The colab explanation is satifactory.
I approve!
@moorepants by "I approve"
, do you mean that you recommend this work is accepted? Thanks again for your help!
@dineshresearch it looks like you have one box unticked: State of the field
:point_up: , can you clarify what is needed to fix that last remaining point? If you have happy to tick the box and recommend that we accept this work please let us know. Thanks!
Yes, I recommend accepting the paper. The only thing would for them to push the latest release to PyPi so that the notebooks work, as mentioned. I assume that would be done in the finalization and zenodo archive, etc.
@dineshresearch can you look at my comment above :point_up: thanks
@dineshresearch please can you pick this review up again? Thanks
@dineshresearch it looks like you have one box unticked: State of the field
:point_up: , can you clarify what is needed to fix that last remaining point? If you have happy to tick the box and recommend that we accept this work please let us know. Thanks!
Submitting author: @wkirgsn (Wilhelm Kirchgässner) Repository: https://github.com/upb-lea/gym-electric-motor Version: v0.3.1 Editor: @Kevin-Mattheus-Moerman Reviewer: @moorepants, @dineshresearch Archive: 10.5281/zenodo.4355691
: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
@moorepants & @dineshresearch, 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 @Kevin-Mattheus-Moerman 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 @moorepants
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @dineshresearch
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper