NelleV
commented
10 years ago Thanks for the submission. The reviews should start soon. Thanks, N
Closed drestebon closed 9 years ago
NelleV
commented
10 years ago Thanks for the submission. The reviews should start soon. Thanks, N
certik
commented
10 years ago Hi, I plan to review this submission. Thanks!
fabianp
commented
10 years ago I will be equally reviewing this submission.
fabianp
commented
10 years ago The authors present SClib, a library built on top of ctypes for wrapping C code.
The paper is well written and the applications are through and well presented. However, the description of the library lacks important information and comparison against similar libraries is non existent.
In the description of the library I find some important omissions. Some questions that potential users might like to know include:
The other weak point of this paper in my opinion is that besides ctypes, the paper does not mention (let alone benchmark) existing solutions to interface C code in Python, such as Swig/Cython/Weave/np_inline. I would be interested to know what are the advantages in terms of speed and flexibility with respect to at least the mainstream way of wrapping C code (Cython in my opinion).
Third, the authors claim that the library has integration with (I)Python. I was hoping that this means the use of any of IPython features such as %magic functions, notebook callbacks or the IPython cluster interface. None of this is mentioned in the paper. In the source code I have not being able to find these features. Because of this, I find it misleading to claim integration with IPython. Of course, since IPython can execute arbitrary code, the libary has the same integration with IPython as any other Python library. I would if the authors could explain what they mean exactly with integration with IPython or correct this statement.
In my opinion, the use of macros of the form PY* should be avoided to avoid clashes with the header python.h
Missing reference {ref{cornell}} at the end of page 2.
In page 3, missing reference eqref{eq:dxdt}
In page 3, "latex-preamble: DeclareMathOperator*{argmin}{arg,min}"
certik
commented
10 years ago I have my review ready, but it needs to be approved by my employer first (which might take about a week or so), thus I should be able to post it by the end of next week.
certik
commented
10 years ago Paper available at: https://github.com/euroscipy/euroscipy_proceedings/pull/39 Commit that was reviewed: b7257d4dd240d9cd008260c68f13a7e47f79e3a2
The authors present a simple library SClib for creating Python wrappers to C functions. From the user perspective, one uses C macros to declare how things should be wrapped on the C level, and then SClib takes care of the rest. The authors then provide two applications of SClib: radial Schrödinger eigensolver and control engineering application.
The second control engineering application does not have enough numerical details in order to reproduce the graphs, but the goal of that section is to provide users with a high level overview of how SClib can be used in control engineering (to efficiently evaluate derivatives and other functions) --- for that purpose the level of detail seems sufficient.
The first application in quarkonium physics contains enough detail in order to reproduce most results. Using the program dftatom described in [1], I have verified that the energies in Table I are correct to all printed digits. I have created an IPython Notebook, available at [2], that uses the Python interface to dftatom and I am providing it as part of this review, so that it is clear exactly how the results were obtained.
Citing from the abstract, the goal of this first application is to “efficiently solve the Schrödinger equation for bounds states” and drive the calculation in IPython (Notebook) so that one can easily manipulate the solutions and visualize results in an interactive environment, without sacrificing speed. Table I shows time comparisons of various radial Schrödinger solvers.
In order to asses the claim whether the speed was sacrificed or not, the authors should compare against other codes that people have used in literature. One could start for example with the list of solvers provided in Table 1 in [1]. In order to get a sense of this, I calculated the four states in Table I using dftatom [1], and with optimized mesh for this problem it seems to be more than 10,000 times faster, i.e. four orders of magnitude. The calculation, plots, and timings are provided in the Notebook [2].
How is the radial mesh determined in the manuscript? I.e. do the timings in Table I include a single fixed mesh calculation (just like it is provided for dftatom in [2]), or does it include adaptive mesh refinement until the solution is converged? In order to make fair comparison with dftatom, one needs to make sure to either: (1) both use the same fixed mesh, or (2) both determine the mesh adaptively.
Even though dftatom in [1] is using a shooting method and bisection, just like the authors used in this paper, one could argue that it is implemented in a more efficient way than the Mathematica code in [3] or the SChroe.py script in this paper. However, given that it is the same method, four orders of magnitude is such a big speedup that the claim that SChroe.py solves the Schrödinger equation efficiently is in my opinion not warranted. The authors should mention this fact, that by implementing the shooting method efficiently in Fortran or C and using a fixed mesh calculation, one can obtain four orders of magnitude speedup. Or they can just add the dftatom’s timings (or of another similarly fast solver) as another column into Table I, assuming that those timings are for a single fixed mesh calculation.
Error in equation: Equation (1) presents Schrödinger equation with reduced mass m, and the term 1/2.
Then the radial equation in (2) must also have the term 1/2. As it is now, the m in equation (2) is twice the m in equation (1), and also the $l(l+1)$ term is missing 1/2. As such, the authors should rename the m in (1) e.g. to mu and clarify that mu=m/2. It is more conventional to use mu, not m. Since the authors use m in their paper, when reproducing their results, it is essential to use mu=m/2 in order to agree.
The graph of the wavefunction in Fig.1 b appears to be incorrectly normalized. Using the usual normalization, the correct graph should look similar to the one in the cell [8] in [2], i.e. notice the different magnitude/normalization.
The paper should also mention how SClib compares against other tools for creating Python wrappers like Cython or Swig.
I noted the following typos/grammatical errors:
E_{n,l} correspond to an eigenvalue” -> correspondsy_{n,l}(r) will diverge except when :math:E_{n,l}
correspond to an eigenvalue.” -> correspondsAnd several formatting issues:
Schr\"odinger doesn’t render correctly, the authors have to write it as Schrödingerref{cornell} doesn’t render correctly. The same on the page 50.ref{coupled}, page 52 doesn’t render correctlylatex-preamble: DeclareMathOperator*{argmin}{arg,min} in the pdf.After addressing the above issues, I think this paper can be accepted.
Ondřej Čertík CCS-2: Computational Physics and Methods Los Alamos National Laboratory LA-UR: LA-UR-14-26771
[1] Čertík, O., Pask, J. E., & Vackář, J. (2013). dftatom: A robust and general Schrödinger and Dirac solver for atomic structure calculations. Computer Physics Communications, 184(7), 1777–1791. doi:10.1016/j.cpc.2013.02.014
[2] http://nbviewer.ipython.org/gist/certik/61bb28bb3198743765c8
[3] Lucha, W., & Schöberl, F. F. (1999). Solving the Schrödinger Equation for Bound States With Mathematica 3.0. International Journal of Modern Physics C, 10(04), 607–619. doi:10.1142/S0129183199000450
pdebuyl
commented
10 years ago Dear @fabianp and @certik , thank you for your reviews.
As @drestebon has updated the manuscript, may you confirm that the updates address satisfactorily your reviews?
Thanks, Pierre
certik
commented
10 years ago @pdebuyl, a lot of my points have not been addressed (looking at the changes: https://github.com/drestebon/euroscipy_proceedings/compare/b7257d4dd240d9cd008260c68f13a7e47f79e3a2...da82c0fa63cb9be4347efd49e6bece7098d24e9f). The authors should write a response where they address all the questions raised in the review and provide justifications for the changes made, point by point. If some correction was not done, they should justify why.
For example I noticed the authors have improved the speed of SChroe.py 16x. They should explain what improvements they did to the code and describe the improvements in the manuscript, or if the previous times were incorrect for some reason, they should explain that in the response.
They have not addressed the questions regarding what exactly is being benchmarked (single mesh vs adaptive refinement) and so on.
drestebon
commented
10 years ago Hello people!
We just commited some more changes regarding the reviews: the existence of other alternatives to embed code in python is addressed, the names of the macros are revised.
Regarding the benchmarking in the particle physics section, Hector is preparing a more detailed explanation, the text is however, in our opinion, ready.
heedmane
commented
10 years ago Hallo, please check the current version of the paper. The explanation of the changes for the referee 1 (@fabianp) are available here: http://users.ph.tum.de/ga98zav/misc/answer_referee1.txt
and for the referee 2 (@certik) here: http://users.ph.tum.de/ga98zav/misc/answer_referee2.txt
the best wishes Hector
fabianp
commented
10 years ago I am satisfied with the response by the authors. In my opinion the paper can be accepted
pdebuyl
commented
10 years ago Reply to referee 1 copied here for reference
The authors present SClib, a library built on top of ctypes for wrapping C code.
The paper is well written and the applications are through and well presented. However, the description of the library lacks important information and comparison against similar libraries is non existent.
Main points
In the description of the library I find some important omissions. Some questions that potential users might like to know include:
• How can I install this library ? Is it available through pypi ? While link to the code in github is present, the README in still opaque in what refers to how the software should be installed and used.
• How should extensions be compiled ? Should they be compiled beforehand or can they be compiled using distutils ? In that case, do they need any special compiler flags (e.g. include directories) ?
The other weak point of this paper in my opinion is that besides ctypes, the paper does not mention (let alone benchmark) existing solutions to interface C code in Python, such as Swig/Cython/Weave/np_inline. I would be interested to know what are the advantages in terms of speed and flexibility with respect to at least the mainstream way of wrapping C code (Cython in my opinion).
Third, the authors claim that the library has integration with (I)Python. I was hoping that this means the use of any of IPython features such as %magic functions, notebook callbacks or the IPython cluster interface. None of this is mentioned in the paper. In the source code I have not being able to find these features. Because of this, I find it misleading to claim integration with IPython. Of course, since IPython can execute arbitrary code, the libary has the same integration with IPython as any other Python library. I would if the authors could explain what they mean exactly with integration with IPython or correct this statement.
Minor points
In my opinion, the use of macros of the form PY* should be avoided to avoid clashes with the header python.h
Missing reference {ref{cornell}} at the end of page 2.
In page 3, missing reference eqref{eq:dxdt}
In page 3, "latex-preamble: DeclareMathOperator*{argmin}{arg,min}"
pdebuyl
commented
10 years ago Reply to referee 2 copied here for reference
Review of “SClib, a hack for straightforward embedded C functions in (I)Python”
Paper available at: #39
Commit that was reviewed: b7257d4
The authors present a simple library SClib for creating Python wrappers to C functions. From the user perspective, one uses C macros to declare how things should be wrapped on the C level, and then SClib takes care of the rest. The authors then provide two applications of SClib: radial Schrödinger eigensolver and control engineering application.
The second control engineering application does not have enough numerical details in order to reproduce the graphs, but the goal of that section is to provide users with a high level overview of how SClib can be used in control engineering (to efficiently evaluate derivatives and other functions) --- for that purpose the level of detail seems sufficient.
The first application in quarkonium physics contains enough detail in order to reproduce most results. Using the program dftatom described in [1], I have verified that the energies in Table I are correct to all printed digits. I have created an IPython Notebook, available at [2], that uses the Python interface to dftatom and I am providing it as part of this review, so that it is clear exactly how the results were obtained.
Citing from the abstract, the goal of this first application is to “efficiently solve the Schrödinger equation for bounds states” and drive the calculation in IPython (Notebook) so that one can easily manipulate the solutions and visualize results in an interactive environment, without sacrificing speed. Table I shows time comparisons of various radial Schrödinger solvers.
In order to asses the claim whether the speed was sacrificed or not, the authors should compare against other codes that people have used in literature.
Our motivation goes beyond the Schroedinger equations solvers since there is plenty of scripts/programs that currently run in platforms that are not only slow but are also closed. We, the authors, believe the principle of reproducibility of the result is essential in science, this principle is in direct confrontation with the use of closed software like Mathematica or Matlab/Simulink in scientific applications and section 3 provides an example of how using the simple interface of SClib we have ported a popular solver from a closed platform to the one provided by (I)Python that offers some of the same advantages of Mathematica. The speed-up we get using SClib allow the authors to do the applications described section 3. Some of these applications, like the parameter fixing including second order corrections, would have being nearly impossible to do with the Mathematica script (or the pure python solver).
One could start for example with the list of solvers provided in Table 1 in [1]. In order to get a sense of this, I calculated the four states in Table I using dftatom [1], and with optimized mesh for this problem it seems to be more than 10,000 times faster, i.e. four orders of magnitude. The calculation, plots, and timings are provided in the Notebook [2].
How is the radial mesh determined in the manuscript? I.e. do the timings in Table I include a single fixed mesh calculation (just like it is provided for dftatom in [2]), or does it include adaptive mesh refinement until the solution is converged? In order to make fair comparison with dftatom, one needs to make sure to either: (1) both use the same fixed mesh, or (2) both determine the mesh adaptively.
Even though dftatom in [1] is using a shooting method and bisection, just like the authors used in this paper, one could argue that it is implemented in a more efficient way than the Mathematica code in [3] or the SChroe.py script in this paper. However, given that it is the same method, four orders of magnitude is such a big speedup that the claim that SChroe.py solves the Schrödinger equation efficiently is in my opinion not warranted.
The authors should mention this fact, that by implementing the shooting method efficiently in Fortran or C and using a fixed mesh calculation, one can obtain four orders of magnitude speedup. Or they can just add the dftatom’s timings (or of another similarly fast solver) as another column into Table I, assuming that those timings are for a single fixed mesh calculation.
Error in equation: Equation (1) presents Schrödinger equation with reduced mass m, and the term 1/2. Then the radial equation in (2) must also have the term 1/2. As it is now, the m in equation (2) is twice the m in equation (1), and also the $l(l+1)$ term is missing 1/2. As such, the authors should rename the m in (1) e.g. to mu and clarify that mu=m/2. It is more conventional to use mu, not m. Since the authors use m in their paper, when reproducing their results, it is essential to use mu=m/2 in order to agree.
The graph of the wavefunction in Fig.1 b appears to be incorrectly normalized. Using the usual normalization, the correct graph should look similar to the one in the cell [8] in [2], i.e. notice the different magnitude/normalization.
The paper should also mention how SClib compares against other tools for creating Python wrappers like Cython or Swig.
I noted the following typos/grammatical errors:
• “The functions are then available as a members of the library” -> “The functions are then available as members of the library” (no “a”)
• in abstract: “conceading” -> “conceding”, but the sentence doesn’t make sense, perhaps the authors meant “compromise”?
• The code for SClib and example use are availible at -> available
• “once the paper appear online” -> “once the paper appears online”
“With the goal of mimic the advantages of this script but without compromising in speed we have developed SChroe.py,” -> “With the goal of mimicking the advantages of this script, but without compromising speed, we have developed SChroe.py” (i.e. two commas missing, “mimicking” and no “in”)
• “The Schrödinger equation is the one the fundamental equations in physics” perhaps the authors meant “one of the fundamental equations” ?
• “we will focus on the time-independent version which in natural units is given by“ -> “we will focus on the time-independent version which, in natural units, is given by”
• “If the relative velocity of between the quark and the antiquark” perhaps remove “between”?
• “the value of :math:E_{n,l} correspond to an eigenvalue” -> corresponds
• “fulfills the condition of have one node” -> “fulfills the condition of having one node”
• “the standard method consist in to apply” perhaps “the standard method consist of applying” sounds better
• “In general :math:y{n,l}(r) will diverge except when :math:E{n,l} correspond to an eigenvalue.” -> corresponds
And several formatting issues:
• Schr\"odinger doesn’t render correctly, the authors have to write it as Schrödinger
• Fig.2: ref{cornell} doesn’t render correctly. The same on the page 50.
• ref{coupled}, page 52 doesn’t render correctly
• section 3 has latex-preamble: DeclareMathOperator*{argmin}{arg,min} in the pdf.
certik
commented
10 years ago Thanks for the reply. I went over it and I think that all my points were sufficiently addressed.
However, I would still add near the equation (2) that m = 2*mu, otherwise it looks like there is a missing factor of 1/2. You are right, that the l(l+1)/r^2 term needed to be divided by 2mu=m.
Otherwise I think it can be merged.
certik
commented
10 years ago One more thing --- I noticed the Table 1 in the left column is overlapping with the text in the right column, at least when I compile it to the pdf myself as seen in this screenshot:
So this should also be fixed.
pdebuyl
commented
10 years ago Hi all, thanks for the authoring and reviewing work! @drestebon @heedmane please address the last comment about m=2 mu. I will take care of the table layout. On my machine it renders almost fine (the "py" of "Schroe.py" goes over the table limit) but is nothing like @certik 's screenshot. I don't know the origin of the problem but I'll make sure that it is fine in the pubished version.
heedmane
commented
10 years ago Hi all, I added the final corrections. I think I solved the problem with the table, please check. From our side the paper is ready.
cheers,
Hector
certik
commented
10 years ago +1 to merge as it is.
pdebuyl
commented
10 years ago Hi all, thanks for the paper and reviews!
The paper is complete and will be merged after routine checks (layout, etc.)
Pierre
pdebuyl
commented
9 years ago Hi @drestebon could you regenerate your figures as pdf? The fact that the labels are in a bitmap format is affecting the print quality of your figures.
drestebon
commented
9 years ago done
We managed to rewrite it in rst. Sorry for the ugly non-zoomable pictures!