Closed whedon closed 2 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @taless474, @hayesall it looks like you're currently assigned to review this paper :tada:.
:warning: JOSS reduced service mode :warning:
Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.
:star: Important :star:
If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿
To fix this do the following two things:
For a list of things I can do to help you, just type:
@whedon commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@whedon generate pdf
Software report (experimental):
github.com/AlDanial/cloc v 1.88 T=0.09 s (969.6 files/s, 99205.1 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Python 86 2298 2106 4781
Markdown 2 27 0 47
TeX 1 3 0 32
make 1 3 0 8
JSON 1 0 0 6
-------------------------------------------------------------------------------
SUM: 91 2331 2106 4874
-------------------------------------------------------------------------------
Statistical information for the repository 'be66f95afd6266cb60f8d390' was
gathered on 2021/07/09.
The following historical commit information, by author, was found:
Author Commits Insertions Deletions % of changes
Eric Winter 192 18132 8947 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
Eric Winter 9185 50.7 8.3 13.41
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.1109/72.712178 is OK
- 10.1007/978-94-017-9816-7 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
I made the first review pass @elwinter. I need some additional time to check functionality, but all of the points below should be possible to do parallel to that.
All of the components are here (software, docstrings, tests), the final step is to make these components accessible to a user. Many of these could be addressed by incorporating a documentation system (e.g. sphinx
) to pull docstrings and present them alongside a high-level overview.
If possible: After writing the high-level documentation; schedule a meeting with a colleague (someone who is fairly comfortable in Python and knows a little about this problem setting), give them links to the software and documentation, and watch them get started with it. This almost always helps reveal theories of the software that are inaccessible.
Excellent. Thanks for your time. I'll start addressing these over the next week or so and resubmit.
Take care Eric
Get Outlook for Androidhttps://aka.ms/ghei36
From: Alexander L. Hayes @.> Sent: Tuesday, July 13, 2021 7:21:22 PM To: openjournals/joss-reviews @.> Cc: Eric Winter @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: nnde: A Python package for solving differential equations using neural networks (#3465)
I made the first review pass @elwinterhttps://github.com/elwinter. I need some additional time to check functionality, but all of the points below should be possible to do parallel to that.
Overall Feedback
All of the components are here (software, docstrings, tests), the final step is to make these components accessible to a user. Many of these could be addressed by incorporating a documentation system (e.g. sphinxhttps://www.sphinx-doc.org/en/master/) to pull docstrings and collect high-level overviews.
If possible: After writing the high-level documentation; schedule a meeting with a colleague (someone who is fairly comfortable in Python and knows a little about this problem setting), give them links to the software and documentation, and watch them get started with it.
Functionality / Installation
Documentation, Testing, and Community Guidelines
Paper Feedback
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/3465#issuecomment-879468265, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABSFWC2YWYSFXXK554RSF3DTXTC7FANCNFSM5AC2IOEQ.
Hey @taless474 how is your review going?
:wave: @hayesall, please update us on how your review is going (this is an automated reminder).
:wave: @taless474, please update us on how your review is going (this is an automated reminder).
@whedon remind @taless474 in 2 weeks
Reminder set for @taless474 in 2 weeks
I made the first review pass at @elwinter's nnde
. As @hayesall did a great job on creating on-point issues, I went through the functionality check first. Executing this:
pip install nnde
git clone https://github.com/elwinter/nnde_demos
cd nnde_demos; python lagaris01_demo.py
(--update
does not work the first time), the demo was successful. However, the demos are located in another repository.
I also ran the tests
and they all pass.
I will test it further after elwinter/nnde#9 is resolved.
Hey @elwinter how is your revision going?
Hi Patrick. Revisions are proceeding. I have closed all but 3 issues opened by the reviewers now, and I hope to have the rest closed by the end of this week.
Take care
Eric
From: Patrick Diehl @.> Sent: Monday, August 9, 2021 10:14 AM To: openjournals/joss-reviews @.> Cc: Eric Winter @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: nnde: A Python package for solving differential equations using neural networks (#3465)
Hey @elwinter https://github.com/elwinter how is your revision going?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/3465#issuecomment-895259410 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSFWCZRWTQIP2HGYUHBGEDT37PBFANCNFSM5AC2IOEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email . https://github.com/notifications/beacon/ABSFWC4PPKH7BIVA7AL563DT37PBFA5CNFSM5AC2IOE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGVOJGEQ.gif
@hayesall The author has addressed most of your concerns. Would you mind to check if you are satisfied.
I'll check this evening. I saw the last main change got cleared over the weekend.
I made the first review pass at @elwinter's
nnde
. As @hayesall did a great job on creating on-point issues, I went through the functionality check first. Executing this:pip install nnde git clone https://github.com/elwinter/nnde_demos cd nnde_demos; python lagaris01_demo.py
(
--update
does not work the first time), the demo was successful. However, the demos are located in another repository.I also ran the
tests
and they all pass. I will test it further after elwinter/nnde#9 is resolved.
@taless474 I think #9 is resolved and you can start to review again.
@hayesall and @taless474 The author has addressed most of your concerns. Would you mind to check if you are satisfied.
Hey @diehlpk I checked off all but "Functionality" from my tasklist a couple days ago (but I didn't want to cause notification noise). I still need a little more time on functionality.
Follow-up: This would be a stronger submission with sphinx/mkdocs and https://github.com/elwinter/nnde/issues/13, but nnde currently meets the baseline requirements.
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@elwinter Can you have a look at elwinter/nnde#13 and sphinx/mkdocs?
@diehlpk Sorry, my previous statement was vague. Those things would be good to have (and should probably be done in the future), but I think this is satisfactory without them.
Update on functionality review: I've used nnde
to reproduce some numerical results, some notes are included here https://github.com/elwinter/nnde_demos/pull/4
@elwinter, thank you for addressing the issues. I started checking the installation and created #22. Would you please take a look at it?
@diehlpk, do we need the demo to be in the same repository? I am not sure if this is an issue and thank you in advance for your response.
@diehlpk, do we need the demo to be in the same repository? I am not sure if this is an issue and thank you in advance for your response.
I think that is not an issue, if it is well documented.
@elwinter How is your revision going?
@taless474 How is your review going?
@elwinter How is your revision going?
@taless474 How is your review going?
@diehlpk, thank you for your comment. We are waiting for @elwinter to address #22, so I can go forward with the next round of review.
@elwinter Could you please fix the issues so Bita can proceed with the review?
@elwinter Could you please fix the issues so Bita can proceed with the review?
Hmm. I thought I had fixed and closed all but one issue a couple of weeks ago, and was waiting for a response on my test coverage question. Did i miss something? Please let me know and i will take care of it.
Thanks Eric
Get Outlook for Androidhttps://aka.ms/ghei36
From: Patrick Diehl @.> Sent: Thursday, October 14, 2021 10:47:07 PM To: openjournals/joss-reviews @.> Cc: Eric Winter @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: nnde: A Python package for solving differential equations using neural networks (#3465)
@elwinterhttps://github.com/elwinter Could you please fix the issues so Bita can proceed with the review?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/3465#issuecomment-943941666, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABSFWC6VT325ZS533WSRFYLUG6I2XANCNFSM5AC2IOEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
@elwinter @taless474 opened that issue https://github.com/elwinter/nnde/issues/22 in your repo. Could you please have a look into that?
@taless474 It seems that @elwinter has fixed the issue and you can proceed with your review.
@diehlpk, I think this is ready. Please proceed with your review/comments, thanks!
@diehlpk, I think this is ready. Please proceed with your review/comments, thanks!
@taless474
Quality of writing: Is the paper well written (i.e., it does not require editing for structure, language, or writing quality)?
Could you please validate if this box should be checked or is still missing?
@diehlpk, I think it can be improved. I didn't check it to ask for your opinion. But overly, it is ready.
@whedon generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@elwinter I briefly read the paper and have the following remarks, so far
I read through and checked off the last box. I think this is in a pretty good state.
@hayesall and @taless474 thanks for finishing the review.
@elwinter Please address the editorial comments and we are good to accept the paper.
@elwinter Please address the editorial comments and we are good to accept the paper.
Hi Patrick. I apologize for the delayed response. As shown below, test coverage (the only remaining issue) is now at 76%. Is this sufficient to continue?
Thanks Eric
Coverage report: 76%[image: Show keyboard shortcuts]
Modulestatementsmissingexcludedcoverage
Total 6341 1508 0 76%
nnde/init.py
I just tested the master branch and got 72% coverage.
This seems fine (I tend to think coverage numbers are more a rule-of-thumb than an absolute threshold).
If you want to report coverage stats on push/pull requests, you can add something like this to the .github/workflows/github-run-nnde-tests.yml
:
- name: Test with pytest
run: pytest --cov=nnde .
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v1
Hi Patrick. I apologize for the delayed response. As shown below, test coverage (the only remaining issue) is now at 76%. Is this sufficient to continue? Thanks Eric Coverage report: 76%[image: Show keyboard shortcuts] Modulestatementsmissingexcludedcoverage Total 6341 1508 0 76% nnde/init.py
0 0 0 100% nnde/differentialequation/init.py 0 0 0 100% nnde/differentialequation/differentialequation.py 6 0 0 100% nnde/differentialequation/examples/init.py 0 0 0 100% nnde/differentialequation/examples/diff1d_half.py 69 0 0 100% nnde/differentialequation/examples/diff1d_halfsine.py 87 0 0 100% nnde/differentialequation/examples/diff1d_halfsine_increase.py 106 0 0 100% nnde/differentialequation/examples/diff1d_one.py 70 0 0 100% nnde/differentialequation/examples/diff1d_zero.py 70 0 0 100% nnde/differentialequation/examples/diff2d_halfsine.py 142 7 0 95% nnde/differentialequation/examples/diff2d_halfsine_increase.py 122 1 0 99% nnde/differentialequation/examples/diff3d_halfsine.py 213 118 0 45% nnde/differentialequation/examples/diff3d_halfsine_increase.py 188 11 0 94% nnde/differentialequation/examples/example_pde1ivp_01.py 53 28 0 47% nnde/differentialequation/examples/lagaris_01.py 13 5 0 62% nnde/differentialequation/examples/lagaris_02.py 13 5 0 62% nnde/differentialequation/examples/tests/init.py 0 0 0 100% nnde/differentialequation/examples/tests/test_diff1d_half.py 263 1 0 99% nnde/differentialequation/examples/tests/test_diff1d_halfsine.py 262 1 0 99% nnde/differentialequation/examples/tests/test_diff1d_halfsine_increase.py 272 1 0 99% nnde/differentialequation/examples/tests/test_diff1d_one.py 263 1 0 99% nnde/differentialequation/examples/tests/test_diff1d_zero.py 263 1 0 99% nnde/differentialequation/examples/tests/test_diff2d_halfsine.py 493 11 0 98% nnde/differentialequation/examples/tests/test_diff2d_halfsine_increase.py 428 1 0 99% nnde/differentialequation/examples/tests/test_diff3d_halfsine.py 92 89 0 3% nnde/differentialequation/examples/tests/test_diff3d_halfsine_increase.py 710 1 0 99% nnde/differentialequation/examples/tests/test_example_pde1ivp_01.py 35 32 0 9% nnde/differentialequation/examples/tests/test_lagaris_01.py 8 5 0 38% nnde/differentialequation/examples/tests/test_lagaris_02.py 8 5 0 38% nnde/differentialequation/ode/init.py 0 0 0 100% nnde/differentialequation/ode/ode.py 3 0 0 100% nnde/differentialequation/ode/ode1.py 5 0 0 100% nnde/differentialequation/ode/ode1ivp.py 27 14 0 48% nnde/differentialequation/ode/tests/init.py 0 0 0 100% nnde/differentialequation/ode/tests/test_ODE.py 9 1 0 89% nnde/differentialequation/ode/tests/test_ODE1.py 12 1 0 92% nnde/differentialequation/ode/tests/test_ODE1IVP.py 7 1 0 86% nnde/differentialequation/pde/init.py 0 0 0 100% nnde/differentialequation/pde/pde.py 3 0 0 100% nnde/differentialequation/pde/pde1.py 5 0 0 100% nnde/differentialequation/pde/pde1ivp.py 29 16 0 45% nnde/differentialequation/pde/pde2.py 5 0 0 100% nnde/differentialequation/pde/pde2diff.py 47 28 0 40% nnde/differentialequation/pde/tests/init.py 0 0 0 100% nnde/differentialequation/pde/tests/test_PDE.py 9 1 0 89% nnde/differentialequation/pde/tests/test_PDE1.py 12 1 0 92% nnde/differentialequation/pde/tests/test_PDE1IVP.py 8 1 0 88% nnde/differentialequation/pde/tests/test_PDE2.py 12 1 0 92% nnde/differentialequation/pde/tests/test_PDE2DIFF.py 7 1 0 86% nnde/differentialequation/tests/init.py 0 0 0 100% nnde/differentialequation/tests/test_DifferentialEquation.py 12 1 0 92% nnde/exceptions/init.py 0 0 0 100% nnde/exceptions/nndeexception.py 2 0 0 100% nnde/exceptions/tests/init.py 0 0 0 100% nnde/exceptions/tests/test_NNDEException.py 7 1 0 86% nnde/math/init.py 0 0 0 100% nnde/math/kdelta.py 2 0 0 100% nnde/math/sigma.py 22 0 0 100% nnde/math/tests/init.py 0 0 0 100% nnde/math/tests/test_kdelta.py 11 1 0 91% nnde/math/tests/test_sigma.py 37 1 0 97% nnde/math/tests/test_trainingdata.py 27 1 0 96% nnde/math/trainingdata.py 41 16 0 61% nnde/neuralnetwork/init.py 0 0 0 100% nnde/neuralnetwork/neuralnetwork.py 8 0 0 100% nnde/neuralnetwork/nnode1ivp.py 250 214 0 14% nnde/neuralnetwork/nnpde1ivp.py 474 425 0 10% nnde/neuralnetwork/nnpde2diff.py 490 448 0 9% nnde/neuralnetwork/slffnn.py 3 0 0 100% nnde/neuralnetwork/tests/init.py 0 0 0 100% nnde/neuralnetwork/tests/test_NNODE1IVP.py 29 1 0 97% nnde/neuralnetwork/tests/test_NNPDE1IVP.py 7 1 0 86% nnde/neuralnetwork/tests/test_NeuralNetwork.py 15 1 0 93% nnde/neuralnetwork/tests/test_PDE2DIFF.py 27 1 0 96% nnde/neuralnetwork/tests/test_SLFFNN.py 9 1 0 89% nnde/trialfunction/init.py 0 0 0 100% nnde/trialfunction/diff1dtrialfunction.py 73 0 0 100% nnde/trialfunction/diff2dtrialfunction.py 79 0 0 100% nnde/trialfunction/diff3dtrialfunction.py 85 0 0 100% nnde/trialfunction/tests/init.py 0 0 0 100% nnde/trialfunction/tests/test_Diff1DTrialFunction.py 57 1 0 98% nnde/trialfunction/tests/test_Diff2DTrialFunction.py 57 1 0 98% nnde/trialfunction/tests/test_Diff3DTrialFunction.py 57 1 0 98% nnde/trialfunction/tests/test_TrialFunction.py 8 1 0 88% nnde/trialfunction/trialfunction.py 3 1 0 67% coverage.py v5.5 https://coverage.readthedocs.io, created at 2021-11-06 10:13 -0400 … On Mon, Nov 1, 2021 at 11:17 AM Patrick Diehl @.***> wrote: @elwinter https://github.com/elwinter Please address the editorial comments and we are good to accept the paper. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3465 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSFWC3G43L6PAECHZKKROLUJ2VO5ANCNFSM5AC2IOEQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
@elwinter Yes, that seems to be fine and this issue is resolved.
@elwinter I briefly read the paper and have the following remarks, so far
* [ ] Please add the city, state, and country to the affiliation * [ ] Provide some reference to PyTorch and TensorFlow 2. In addition, I think the related work section is really sparse and some mention of other packages and how they differ would be beneficial for the reader. * [ ] You mention that FEM and FDM can be difficult to parallelize. Can you provide some reference for that claim?
@winter I think these are the last open items for the paper.
Ok are there issues open for them, or are they to be handled outside the issue flow? Thanks Eric
Get Outlook for Androidhttps://aka.ms/ghei36
From: Patrick Diehl @.> Sent: Sunday, November 7, 2021 3:19:31 PM To: openjournals/joss-reviews @.> Cc: Eric Winter @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: nnde: A Python package for solving differential equations using neural networks (#3465)
@elwinterhttps://github.com/elwinter I briefly read the paper and have the following remarks, so far
[ ] Please add the city, state, and country to the affiliation
[ ] Provide some reference to PyTorch and TensorFlow 2. In addition, I think the related work section is really sparse and some mention of other packages and how they differ would be beneficial for the reader.
[ ] You mention that FEM and FDM can be difficult to parallelize. Can you provide some reference for that claim?
@Winterhttps://github.com/Winter I think these are the last open items for the paper.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/3465#issuecomment-962674280, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABSFWC4Y6MD7IZFBM2K3NZTUK3NNHANCNFSM5AC2IOEQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
Submitting author: @elwinter (Eric Winter) Repository: https://github.com/elwinter/nnde Version: v1.0 Editor: @diehlpk Reviewer: @taless474, @hayesall Archive: 10.5281/zenodo.5879387
: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
@taless474 & @hayesall, 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 @diehlpk 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 @taless474
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
Review checklist for @hayesall
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper