sympy / sympy-paper

Repo for the paper "SymPy: symbolic computing in python"
https://peerj.com/articles/cs-103/
Other
48 stars 40 forks source link

Peer reviews checklist #204

Closed asmeurer closed 7 years ago

asmeurer commented 7 years ago

(feel free to edit this checklist)

editor

reviewer 1

reviewer 2

reviewer 3

Specific comments:

   >>> (exp(100)+1).evalf() - exp(100).evalf()
   0
   >>> ((exp(100)+1) - exp(100)).evalf()
   1.00000000000000
   >>> (exp(100)+1) - exp(100)
   1
or two different ways to compute the 100th Fibonacci number:
   >>> phi = (1+sqrt(5))/2
   >>> psi = (1-sqrt(5))/2
   >>> ((phi**100-psi**100)/sqrt(5)).evalf()-fibonacci(100)
   65536.0000000000
   >>> ((phi**100-psi**100)/sqrt(5) - fibonacci(100)).evalf()
   0.e-104

Obviously, like your own example, these are problematic because a part of the computation is relying on Python’s builtin floats. Please include a comment on whether symbolic computation (i.e., applying evalf() on the whole expression) always avoids these errors or not, possibly with an example when it doesn’t resolve this problem.

     >>> import os
     >>> os.environ[’SYMPY_DEBUG’] = ’True’
     >>> from sympy import *
     >>> x = symbols(’x’)
     >>> limit(sin(x)/x, x, 0)
     DEBUG: parsing of expression [(0, 1, None, None)] with symbol _w
     DEBUG: returned None
     DEBUG: parsing of expression [(_w, 1, None, None)] with symbol _w
     DEBUG: returned ([], [(_w, 1, None, None)], 1, False)
     DEBUG: parsing of expression [(0, 1, None, None)] with symbol _w
     DEBUG: returned None
     DEBUG: parsing of expression [(_w, 1, None, None)] with symbol _w
     DEBUG: returned ([], [(_w, 1, None, None)], 1, False)
     DEBUG: parsing of expression [(_w, 1, None, None)] with symbol _w
     DEBUG: returned ([], [(_w, 1, None, None)], 1, False)
     DEBUG: parsing of expression [(0, 1, None, None)] with symbol _w
     DEBUG: returned None
     DEBUG: parsing of expression [(_w, 1, None, None)] with symbol _w
     DEBUG: returned ([], [(_w, 1, None, None)], 1, False)
     DEBUG: parsing of expression [(0, 1, None, None)] with symbol _w
     DEBUG: returned None
     DEBUG: parsing of expression [(_w, 1, None, None)] with symbol _w
     DEBUG: returned ([], [(_w, 1, None, None)], 1, False)
     DEBUG: parsing of expression [(1, 1, None, None)] with symbol _w
     DEBUG: returned None
     limitinf(x*sin(1/x), x) = 1
     +-mrv_leadterm(_p*sin(1/_p), _p) = (1, 0)
     | +-mrv(_p*sin(1/_p), _p) = ({_p: _Dummy_14}, {}, _Dummy_14*sin(1/_Dummy_14))
     | | +-mrv(_p, _p) = ({_p: _Dummy_14}, {}, _Dummy_14)
     | | +-mrv(sin(1/_p), _p) = ({_p: _Dummy_15}, {}, sin(1/_Dummy_15))
     | |   +-mrv(1/_p, _p) = ({_p: _Dummy_15}, {}, 1/_Dummy_15)
     | |     +-mrv(_p, _p) = ({_p: _Dummy_15}, {}, _Dummy_15)
     | +-rewrite(_Dummy_14*sin(1/_Dummy_14), {exp(_p): _Dummy_14}, {}, _p, _w)
             = (sin(_w)/_w, -_p)
     | | +-sign(_p, _p) = 1
     | | +-limitinf(1, _p) = 1
     | +-calculate_series(sin(_w)/_w, _w) = 1
     |   +-limitinf(_w, _w) = oo
     |   | +-mrv_leadterm(_w, _w) = (1, -1)
     |   | | +-mrv(_w, _w) = ({_w: _Dummy_18}, {}, _Dummy_18)
     |   | | +-rewrite(_Dummy_18, {exp(_w): _Dummy_18}, {}, _w, _w) = (1/_w, -_w)
| |||+-sign(_w,_w)=1
     |   | | | +-limitinf(1, _w) = 1
     |   | | +-calculate_series(1/_w, _w) = 1/_w
     |   | +-sign(-1, _w) = -1
     |   | +-sign(1, _w) = 1
     |   +-limitinf(_w, _w) = oo
     |   +-limitinf(_w, _w) = oo
     |   +-limitinf(_w, _w) = oo
     +-sign(0, _p) = 0
     +-limitinf(1, _p) = 1

Emphasize that the environment variable SYMPY_DEBUG must be set before importing SymPy for the first time.

asmeurer commented 7 years ago

Regarding reviewer 1, point 2, I agree that [8] is a bad reference, because we are not actually using their definition. The aim was just to have a reference for "simplification is not well-defined", but that paper aims to define it. What is a better reference here? Perhaps Moses [19] from the Carette paper? Perhaps we can remove the reference (the referenced statement is "it must be emphasized that simplification is not a rigorously defined mathematical operation").

asmeurer commented 7 years ago

Regarding reviewer 2, point 4, about assumptions, the best reference for assumptions is http://docs.sympy.org/latest/modules/core.html#sympy.core.assumptions. assumptions0 is internal, and it doesn't list all assumptions anyway. Attributes that start with is_ would be good if we didn't also have all the is_Integer and whatnot. If the best place is the documentation, should we link it in the paper?

asmeurer commented 7 years ago

Some notes on the editor comments (see above):

2: I'm unclear what exactly the editor is asking here. Does anyone have any insights?

3: This indeed doesn't work. We can mention it in the rebuttal. I have opened a SymPy issue here.

(side note, if you want to reference numbers at the beginning of a line in markdown you have to format it in a way that it doesn't think it's a numbering, or else it will renumber them starting from 1)

asmeurer commented 7 years ago

Some notes on reviewer 3 comments (see above):

3: I recall we looked this up and the spelling consensus agreed with the way we have it. We should double check (perhaps we can email the editor to ask if the journal has a convention).

5: We need to check with the Jupyter folks on what their preferred citation is.

7: We had decided not to use first person. We should perhaps email the editor to ask about this.

9: Line 119 is the output of the input from line 118 (for reference, there are the lines)

118 >>> (x**2 - 2*x + 3)/y
119 (x**2 - 2*x + 3)/y

so not showing it would be incorrect. The aim here was simply to show that the input expression remains unevaluated. If anyone has any suggestions on how to improve this presentation, let me know. Perhaps we can mention that the output remains symbolic. At any rate, this also seems worth a mention in the rebuttal letter.

13: Not sure I agree with this, but it's a minor point.

16: This was already fixed in https://github.com/sympy/sympy-paper/pull/203 via reviewer 1 point 6 (not that exact place, but I think it is equivalent)

22: Let's replace "2D" with "multiline" or something similar (or explain what is meant by 2D).

25: I'm unclear why this example might be better. I suppose it is simpler. Perhaps @fredrik-johansson can comment if he approves of this change.

37: I disagree with this. "submodules" specifically indicates that they are subpackages (in the Python sense) of the sympy top-level package. The Python documentation seems to agree with this convention (see https://docs.python.org/3/tutorial/modules.html#packages and https://docs.python.org/3/glossary.html).

41: There is a url in the bib entry. How do we make it appear in the citation?

asmeurer commented 7 years ago

Regarding editor comment 8, the editor for the article is in the bibtex file. The bibliographystyle used by the template is apalike.

Regarding the citation for Jupyter, I have asked them here.

asmeurer commented 7 years ago

Regarding the list of assumptions, the way to do it in code is sys.modules['sympy.core.assumptions']._assume_defined (you can't even use sympy.core.assumptions._assume_defined because of sympy.core.core). Clearly this isn't something we want to recommend in any way, especially when there is a page in the documentation.

ashutoshsaboo commented 7 years ago

Hi @asmeurer . I guess we must also move the SymPy Live section to the main paper. It'll provide a very good base for testing all the examples that we give ahead (in the paper) further.

asmeurer commented 7 years ago

I would just mention SymPy Live (a single sentence), perhaps in the intro or at the top of the architecture section near the first example.

asmeurer commented 7 years ago

Regarding SageMath, http://www.sagemath.org/library-publications.html#CiteSage and https://wiki.sagemath.org/Publications_using_SageMath?action=show&redirect=Publications_using_SAGE give a bibtex entry. It is a software citation, which would mean replacing the existing paper citation with a software citation, which is somewhat unfortunate, but I think it's fine. It also says we should contact them (I guess so they can put it on that page), so I will do that as well.

asmeurer commented 7 years ago

I guess we just need to make a PR against https://github.com/sagemath/publications/

asmeurer commented 7 years ago

Regarding reviewer 3, point 46 (about the SymPy Gamma screenshot), what do people think? Sergey has added a note that content is rendered as html, which I agree with (we should not re-render it as LaTeX). But we also might want to just remove it altogether. It does make the technical aspects of submitting the paper easier if there are no figures 😉