sympy / sympy-paper

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

Minor revisions #215

Closed asmeurer closed 7 years ago

asmeurer commented 7 years ago

The reviews are back for minor revisions:

Editor comments

Reviewer 1

(there do not appear to be any comments from reviewer 1)

Reviewer 2

Basic reporting No comments.

Experimental design No comments.

Validity of the findings No comments.

Comments for the author This is a great paper. Thank you for addressing my concerns.

Reviewer 3

Basic reporting The structure of the paper and the supplement is now improved, compared to the original submission, and the materials are now much easier to read and comprehend.

Experimental design No Comments

Validity of the findings No Comments

Comments for the author The paper

The supplement

asmeurer commented 7 years ago

Some thoughts:

Editor comment 1: I guess we need to reword this somehow. I think the editor is misunderstanding our use of the term "identity". An equation A = B is an identity on some domain (say, for complex valued variables) if it holds for all values in that domain. Saying sqrt(t**2) = t is an identity for complex t because it holds for some complex t doesn't make sense. Any suggestions on how to reword this? I do want to leave it in, as it's the #1 (by far) real world example of the assumptions being needed to simplify an expression.

Review comment on the supplement figure 1: The figure code is generated automatically from the category theory module. I guess this should be made clearer in the caption (like "A diagram typeset in Xy-pic automatically by XypicDiagramDrawer").

Reviewer comment for supplement figure 2: Both links will show the integral, but it's probably better to actually use the integral link, as that's less confusing.

The rest of the changes are straightforward.

certik commented 7 years ago

Regarding Editor comment 1, I propose a fix that I pushed into the peer-reviews branch in ceb444000109e4bda80c846ab9e52cadcb24b1de. The way it was written was indeed confusing, as the word identity was used both for an equation that only holds for some t as well as for an equation that holds for all complex t.

asmeurer commented 7 years ago

I see. I didn't notice that we used the word "identity" the first time. That is indeed wrong. I like your change (although see my comment), but it doesn't address the "how do you define square root" issue. Should we add a footnote indicating that "\sqrt{t} is the principle square root as usually defined, with a branch cut on the negative real axis" or something similar?

certik commented 7 years ago

@asmeurer I've indicated that a bit in the footnote, but you are right that I should also indicate that that identity assumes the sqrt is itself defined on the usual principal branch. I clarified it in ece7a3fa4cec511a8e66e235c43a97d29674aa06.

Regarding how sympy does it ---- does sympy assume the usual principal branch? I would think so, since that's what's used when you evaluate numerically. As well as in Python and other languages.

asmeurer commented 7 years ago

Yes as far as I am aware sympy uses the usual defaults that you would expect. I just checked and sqrt(I) is in the first quadrant. I know the one that trips people up sometimes is that root(-1, 3) gives a complex number.