Closed whedon closed 5 years ago
Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @mrava87, it looks like you're currently assigned as the reviewer for this paper :tada:.
: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
Attempting PDF compilation. Reticulating splines etc...
PDF failed to compile for issue #1356 with the following error:
% Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
100 13 0 13 0 0 254 0 --:--:-- --:--:-- --:--:-- 260
Can't compile the PDF, the bibtex file has 3 extraneous references: @Rang:2015, @Cellier:2018, @Leveque:2002
Hi @celliern, nice library. Here are some comments. As they are all quite general I have not created issues on the repo yet. Please have a look and let me know if everything is clear and if you agree with the suggested points and I will create some specific issues for those tasks you agree you will be able to tackle.
General comments:
Hook
but I am not sure why I would call this a non-linear effect then as this is linear. Can you comment?Paper: fix the following typos
Installation: I don’t see packages in scikit-fdiff conda channel that you mention in the documentation.
Examples:
>>>>
. I suggest to at least remove those >>>>
. A better solution could be to add links to python scripts (or better jupyter-notebooks) and even better to convert them into sphinx-gallery examples which are run as part of readthedoc build project. I see two main advantages by implementing this last option: i) you are entirely sure your examples will always run, typos or changes in code that you forget to fix in the example will be easy to spot as your documentation build would crash otherwise, ii) everyone can download python files or notebooks with the example and run it.Contribution: I can’t find any info related to how to contribute, https://scikit-fdiff.readthedocs.io/en/latest/contribute.html# is empty.
Documentation API: I see that your documentation, especially is very accurate in the going further section. However it looks to me a bit more like a long story (paper style) other than a software documentation. I would strive to include also a skdiff API
section where users have easy to all main classes and functions in your library that you want to expose to them. If for example I want to use fft routines in numpy but I do not need to understand the theory behind as I am familiar I will go here https://docs.scipy.org/doc/numpy/reference/routines.fft.html, I feel something along this lines is lacking in your documentation. As you use sphinx it is very easy to add something like that, if you don’t know how to do it I can point you to some examples :)
Versioning: I am not sure this project follows semantic versioning. You say that v0.6.0 is a huge step for the software, does this make it incompatible with previous versions (any backward incompatible change being made?). I would think that if the software name changed all the imports in previous scripts will not work anymore? In that case is very important to boost the first number (here would be v1.0.0). I am not sure how to go about here but I think you should aware and at least strive for that from here onwards.
News / TODO / Roadmad: suggest to move this to independent md file and/or in documentation
That's all for me, really nice to discover this library!
MR
Hi @mrava87 ! Thanks for the review !
I will answer to the different points :
model = Model("dxxU - 1/c**2 dxxU - f", "U(x)", parameters=["f"])
then providing f
when initializing the fields. You can also explicitly write "f" if you have it as a function of x.
model = Model("dxxU - 1/c**2 dxxU - f", "U(x)", subs={"f": "cos(x)"})
for example.I will also do the typo fixes for the paper.
Thank a lot for all these remarks !
Nicolas Cellier
No problem :) Let me know when you have added things and I will take another look
Just a quick follow up:
- Second order time derivative can be solved with using an intermediate variable and solving two first order equation instead of one second order equation.
I assumed that would have been the case but wasn't sure if you could do it with second order time derivative as well, good to know :)
- You can implement external force by writing them explicitly as
model = Model("dxxU - 1/c**2 dxxU - f", "U(x)", parameters=["f"])
then providingf
when initializing the fields. You can also explicitly write "f" if you have it as a function of x.model = Model("dxxU - 1/c**2 dxxU - f", "U(x)", subs={"f": "cos(x)"})
for example.
Maybe I wasn't precise enough, I meant a time-space dependent external forcing f
, for example say I want to implement this set of equations:
dtVx = -dxP/rho,
dtVy = -dyP/rho,
dtP = -dxVx/k - dyVy/k +q,
where q(t, x, y)
, can you do that?
You can do that in two way :
If you have the explicit q
function, you can define put it in the model :
model = Model(["-dxP / rho", "-dyP / rho", "-dxVx / k - dyVy / k + cos(t) * sin(y) + x**2"],
["Vx", "Vy", "P"],
parameters=["rho", "k"])
or use the subs argument to lighten a bit the writting
model = Model(["-dxP / rho", "-dyP / rho", "-dxVx / k - dyVy / k + q"],
["Vx", "Vy", "P"],
parameters=["rho", "k"],
subs={"q": "cos(t) * sin(y) + x**2"})
Or, if you cannot write q
as a sympy expr, you can provide it as a parameters, then update it using a hook.
model = Model(["-dxP / rho", "-dyP / rho", "-dxVx / k - dyVy / k + q"],
["Vx", "Vy", "P"],
parameters=["rho", "k", "g"])
def compute_q(t, fields):
fields["q"] = some_stuff_to_process_g(t, fields)
return fields
simulation = Simulation(..., hook=compute_q)
Nice :) I followed this and got an example working but I think there are still some issues with dispersion due to source injection. Let's take this off this issue and do it here https://gitlab.com/celliern/scikit-fdiff/issues/2
Hello again ! A word to say that most of the point addressed have been fixed. There is just the creation of the cookbook that is missing, but you can already look at the new version of the documentation if you want.
It will be in the dev branch :
Great :)
Looks very good to me. Just one minor thing:
News / TODO / Roadmad
etc. from the list of index in the README as now their link is broken.Congrats with a nice software and documentation. This is good to go for me when you get the cookbook and adapt the paper (don't see it the pdf here)
@poulson can you please also contribute your side of the review? Thanks!
@Kevin-Mattheus-Moerman Please excuse the delay, I am getting back to this now!
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
Thank you for your patience, Nicolas (@celliern).
First, some comments on the paper:
Some comments on the code itself:
Hello !
No problem : the review process is still way faster than any "classic" journal.
Note taken for the references : I will add references to the Fenics project, and better, say a word on software targeting the same goals and their difference with my project. I will add citation for sympy as well as other main dependencies as well.
For the computational resources, it's a bit more than just the sparsity : as the system is entirely discretized with sympy, the numerical routines (F and J) are as simple as possible, making them really fast (even with the default numpy backend). In addition, the vectorization chosen for the repartition of the unknown lead to an "optimal" system to solve (as decribed here). Moreover, using by default Rosenbrock Wanner schemes allow to have only one system factorization for one time upgrade, drastically reducing the numerical cost. My bad, I should have add that to the paper. I will be done.
Thanks to the spelling, they will be fixed soon.
For the examples, I am building a gallery as @mrava87 already suggests. The 1D / 2D dam break will be present as well as more complex cases.
I note the suggestion for the docstring. I will add the docstring for all public methods (but as being solo for this project for now, this will take some time...).
For the limiting of 79 characters, I chose to use the black formatting that set the limit to pep8 + 10%, and their reasonning can be found here.
Thank you for the review. I will modify the paper and create the gallery as fast as I can (but being in vacation, I cannot work as fast as I would).
As complement on the examples, you can find two complex working examples (that will be in the gallery) here :
https://nbviewer.jupyter.org/gist/celliern/c9f408c905b2cd533dcb7c7ffbe9c851
The first one has been built by @mrava87, the second one is a 2D dam break problem.
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
Hello again!
Sorry for the delay !
I have updated the repository as well as the doc with the different suggestions, and I have added some examples and validation cases to a cookbook.
https://gitlab.com/celliern/scikit-fdiff/ https://scikit-fdiff.readthedocs.io/ https://scikit-fdiff.readthedocs.io/en/latest/auto_examples/index.html
@mrava87 @poulson
Hi, nice to see all the changes you implemented. I am happy to recommend this for publication.
Just one small comment regarding the example One dimension shallow water, dam break case.
I struggle to see the equation that you write in the implementation. For example the first eq has one term (plus the time deriv)... ignore this comment if you think I am wrong :)
I am also very happy to see the added examples: I believe the package is significantly improved as a result.
I would also add the small comment that the timings for the 2D acoustic wave and dam examples are both listed as 0 seconds; it would be great if timings on a commodity laptop or workstation were added.
EDIT: I should also add that a short 'How to contribute' section in the README, or a 'CONTRIBUTING' file, would be ideal.
Other than that, I am happy to recommend this for publication as well.
@mrava87 You are right, they should be equivalent, but the notation differs and leads to confusion. I will fix it.
@poulson I will "clone" the doc contribute to the readme, as you suggest. The 0 timing is due to the rtd limitation: I cannot generate "huge" case in their servers. To be coherent, I will benchmark all the simulation cases on laptop / desktop and small cluster (4 / 8 / 32 cpu, with specs) to have a good idea. Update very soon !
@celliern let us know when you've implemented all changes so we can resume review. Below are some points to consider.
[x] In terms of contributing guidelines, have you added anything yet? I recommend creating a CONTRIBUTING.md file and linking to it in the README.md file with a nice heading. There are guidelines and templates for creating such a file available too (here is one of mine).
[x] It is also good practice to add a COC.md (code of conduct) file and link to it in the README.md file too. There is a template for that here: https://www.contributor-covenant.org/
@Kevin-Mattheus-Moerman @mrava87 @poulson
Hello!
I did the requested changes: a proper benchmark of the examples, some fix in the equations. I have also linked the contrubute section of the doc in the readme and added a code of conduct.
The changes live in the dev branche
https://gitlab.com/celliern/scikit-fdiff/tree/dev https://scikit-fdiff.readthedocs.io/en/dev/index.html
Great. @celliern. Minor point, I noticed your README says "See the contribe section of the doc" which should probably read: "See the contribute section of the doc"
@mrava87 @poulson can you review these updates
I did. Everything looks good to me, thanks for updating the documentation :)
I am happy to recommend this for publication as well. @Kevin-Mattheus-Moerman
Hello @Kevin-Mattheus-Moerman !
I have fixed the typo you noticed, and updated the master branch to the repo. Is there anything left I can do?
@celliern no thank for alerting me. I'll run a few check and will let you know if anything needs to be done
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
@whedon check references
Attempting to check references...
OK DOIs
- 10.1016/j.cam.2015.03.010 is OK
- 10.7717/peerj-cs.103 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@celliern below are suggested changes to your paper. I tried to improve the language, rephrase some difficult sentences, and fixed several typos. Can you please check it thoroughly to make sure I did not misunderstand/break what you tried to say in some of your sentences. If you agree with these changes please paste them into your paper.md file. I also suggest you add a sentence to the last paragraph to hint at applications. Currently it suddenly mentions "the shallow water equation". Perhaps you can start it with something like: "Scikit-FDiff can be used for a wide range of application including A, B and C. To date the authors have used it for solving X type of problems. Furthermore, Scikit-Fdiff has been validated for solving the shallow-water equation on dam-breaks ..."
Scikit-FDiff is a new tool for Partial Differential Equation (PDE) solving, written in pure Python, that focuses on reducing the time between the development of the mathematical model and the numerical solving.
It allows easy and automated finite difference discretization, thanks to symbolic processing (using the SymPy library [@Meurer:2017]) which can deal with systems of multi-dimensional partial differential equations and complex boundary conditions.
Using finite differences, and the method of lines, Scikit-FDiff allows for the transformation of the original PDE into an Ordinary Differential Equation (ODE), providing fast computation of the temporal evolution vector and the Jacobian matrix. The latter is pre-computed in a symbolic way and is sparse by nature. An efficient vectorization allows one to formulate the numerical system in such a way as to facilitate the numerical solver work, even for complex multi-dimensional coupled cases. Systems can be evaluated with as few computational resources as possible, enabling the use of implicit and explicit solvers at a reasonable cost.
Scikit-FDiff stands out in comparison to other competitors, such as the FEniCS Project, Clawpack, or the Dedalus Project, in terms of its simplicity. Despite the fact that Scikit-FDiff uses a relatively simple method (finite-differences), which allows one to code in a way which is close to the mathematical model, it is able to solve real-world problems. It is however less suited than other packages for building specialized solvers.
Scikit-FDiff also contains several classic ODE solver implementations (some of which have been made available from dedicated python libraries), including the backward and forward Euler scheme, Crank-Nicolson, and explicit Runge-Kutta. More complex approaches, like the improved Rosenbrock-Wanner schemes [@Rang:2015] up to the 6th order, are also available in Scikit-FDiff. The time-step is managed by a built-in error computation, which ensures the accuracy of the solution.
The main goal of this software is to minimize the time spent writing numerical solvers allowing one to focus on model development and data analysis. Therefore, Scikit-Fdiff has been formulated such that it can solve both simple examples and complex models in only a few line of code. In addition, extra tools are provides, such as data saving during the simulation, real-time plotting, and post-processing.
Scikit-Fdiff has been validated with the shallow-water equation on dam-breaks [@Leveque:2002] and the steady-lake case. It has also been applied to heated falling-films [@Cellier:2018], droplet spread, and simple moisture flow in a porous medium.
@mrava87, @poulson thanks for reviewing this submission! :tada:
@mrava87 are you able to tick those last boxes?
Done, thanks @Kevin-Mattheus-Moerman for assisting us in the process and congrats @celliern
@Kevin-Mattheus-Moerman done for the paper modification ! Thanks @poulson and @mrava87 for the review and @Kevin-Mattheus-Moerman for the edition !
@whedon generate pdf
Attempting PDF compilation. Reticulating splines etc...
PDF failed to compile for issue #1356 with the following error:
/app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-379fcd917139/lib/whedon.rb:114:in check_fields': undefined method
keys' for #initialize' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-379fcd917139/lib/whedon/processor.rb:36:in
new'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-379fcd917139/lib/whedon/processor.rb:36:in set_paper' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-379fcd917139/bin/whedon:55:in
prepare'
from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in
invoke_command'
from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in
start'
from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-379fcd917139/bin/whedon:116:in <top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in
load'
from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in `
@arfon do you know what's going wrong here?
@Kevin-Mattheus-Moerman - it looks like the paper isn't structured for JOSS properly https://gitlab.com/celliern/scikit-fdiff/blob/master/paper.md
See https://joss.readthedocs.io/en/latest/submitting.html#example-paper-and-bibliography on how the paper should be structured.
@celliern the paper is not being generated because you deleted this from the top, please put it back:
---
title: 'scikit-finite-diff, a new tool for PDE solving'
tags:
- python
- pde
- physical modelling
authors:
- name: Nicolas Cellier
orcid: 0000-0002-3759-3546
affiliation: 1
- name: Christian Ruyer-Quil
affiliation: 1
affiliations:
- name: Université Savoie Mont-Blanc
index: 1
date: 18 March 2019
bibliography: paper.bib
---
Also I noticed you removed the references to [@Leveque:2002]
(dam breaks) and [@Cellier:2018]
(heated falling-films). It is good to comment on the use of the software with such references so I would encourage you to put these back unless you have a good reason to remove them.
@celliern In relation to the paper, I created a merge request here with proposed changes to the paper: https://gitlab.com/celliern/scikit-fdiff/merge_requests/3 can you check that out?
As you can see I also added an Acknowledgement, and a Reference heading. The References will be added automatically but you need to add text for the acknowledgements.
Submitting author: @celliern (Nicolas Cellier) Repository: https://gitlab.com/celliern/scikit-fdiff/ Version: v0.6.0 Editor: @Kevin-Mattheus-Moerman Reviewer: @mrava87, @poulson Archive: 10.5281/zenodo.3236970
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) 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
@mrava87 & @poulson, 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.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @Kevin-Mattheus-Moerman know.
✨ Please try and complete your review in the next two weeks ✨
Review checklist for @mrava87
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?Review checklist for @poulson
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?