Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: TaylorSeries.jl: Taylor expansions in one and many variables in Julia #1043

Closed
whedon opened this Issue Oct 22, 2018 · 134 comments

Comments

Projects
None yet
8 participants
@whedon
Copy link
Collaborator

whedon commented Oct 22, 2018

Submitting author: @lbenet (Luis Benet)
Repository: https://github.com/JuliaDiff/TaylorSeries.jl
Version: v0.9.2
Editor: @jedbrown
Reviewer: @sriharikrishna, @tobydriscoll
Archive: 10.5281/zenodo.2628898

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/8ca98df42c598c151f917b6e883be8b3"><img src="http://joss.theoj.org/papers/8ca98df42c598c151f917b6e883be8b3/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/8ca98df42c598c151f917b6e883be8b3/status.svg)](http://joss.theoj.org/papers/8ca98df42c598c151f917b6e883be8b3)

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

@sriharikrishna & @tobydriscoll, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @jedbrown know.

Please try and complete your review in the next two weeks

Review checklist for @sriharikrishna

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: v0.9.2
  • Authorship: Has the submitting author (@lbenet) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

Review checklist for @tobydriscoll

Conflict of interest

Code of Conduct

General checks

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: v0.9.2
  • Authorship: Has the submitting author (@lbenet) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?
@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Oct 22, 2018

@sriharikrishna @tobydriscoll 👋 Welcome! The comments from whedon above outline the review process. I'll be watching this thread if you have any questions. Thanks!

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Oct 22, 2018

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @sriharikrishna, it looks like you're currently assigned as the reviewer for this paper 🎉.

⭐️ Important ⭐️

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:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Oct 22, 2018

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Oct 22, 2018

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Nov 23, 2018

@sriharikrishna @tobydriscoll 👋 I just wanted to check in to see if you need anything in order to complete your review soon.

@tobydriscoll

This comment has been minimized.

Copy link
Collaborator

tobydriscoll commented Nov 24, 2018

@tobydriscoll

This comment has been minimized.

Copy link
Collaborator

tobydriscoll commented Nov 30, 2018

It's excellent software, well documented and with all the required elements in place. I have just minor (very picky) observations:

  1. There should be some pointers to other automatic differentiation software for Julia. It's not hard to find, but links are appreciated.
  2. The display of exponents is partially obscured for me in Juno. It's find at a command shell.
  3. Maybe reconsider exporting the names "gradient" and "hessian", as those are not uncommon from other packages too. But I recognize that namespace is becoming a thorny Julia problem.
  4. The function names "integrate" and "derivative" are verb and noun. I would have preferred they both be one or the other. (I did say I was being picky here.)
@lbenet

This comment has been minimized.

Copy link

lbenet commented Dec 5, 2018

Thanks a lot @tobydriscoll for your review and observations, and sorry for my late reaction; I was traveling.

Some comments on your observations (keeping the same order):

  1. Is the suggestion to add those links in the README.md file, the documentation, or in the paper?
  2. I simply can't reproduce the problem. I think it may be an issue related to the fonts you are using.
  3. We will follow your suggestion not to export those functions; indeed, these functions are common in other packages.
  4. We are dealing with this. While we agree with your observation (and are opting for verbs) other packages use precisely those names: ForwardDiff.jl uses derivative and SymPy.jl uses integrate. Our current idea is to keep derivative, adding diff and differentiate as verbs. We will also maintain integrate, and perhaps add integ.
@lbenet

This comment has been minimized.

Copy link

lbenet commented Jan 9, 2019

@tobydriscoll We have just merged JuliaDiff/TaylorSeries.jl#188, which addresses your comments.

The concrete changes are the following:

  • We added links to other Julia packages related to automatic differentiation in the documentation.
  • We are not exporting any more gradient, jacobian, hessian, jacobian! and hessian! as suggested. A new version will be released that includes these changes.
  • We are keeping derivative but added differentiate.
  • The documentation was updated.

Regarding to point 2 in your review, we couldn't reproduce the problem. It may be related to the fonts you have, or to the specific platform; if you can provide us with more details, we could address the problem.

Thanks a lot for the review. If you have further suggestions, please let us know them.

cc @jedbrown

@lbenet

This comment has been minimized.

Copy link

lbenet commented Jan 24, 2019

@jedbrown I'd like to mention that we released today v0.8.1 of our package. I hope this is ok with the current review process.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Jan 24, 2019

@lbenet Certainly!

@sriharikrishna How is your review going?

@sriharikrishna

This comment has been minimized.

Copy link
Collaborator

sriharikrishna commented Feb 1, 2019

@jedbrown @lbenet I am very sorry for the inordinate delay.

I was able to review the software and confirm its functionality. The requirements for acceptance are met fully. I was unable, however, to check the performance claims because I do not have Mathematica readily available.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Feb 1, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 1, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 1, 2019

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Feb 1, 2019

@whedon check references

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 1, 2019

Attempting to check references...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 1, 2019


OK DOIs

- http://doi.org/10.1137/141000671 is OK
- http://doi.org/10.1145/1236463.1236468 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Feb 1, 2019

Thanks @sriharikrishna!

@lbenet Please see my PR referenced above. Once merged, I'll re-check the paper and if all looks good, we'll be ready to archive. Thanks.

@lbenet

This comment has been minimized.

Copy link

lbenet commented Feb 2, 2019

Thanks a lot @sriharikrishna.

@jedbrown There is a small typo on a keyword in JuliaDiff/TaylorSeries.jl#192. Aside from that, LGTM.

@lbenet

This comment has been minimized.

Copy link

lbenet commented Feb 4, 2019

JuliaDiff/TaylorSeries.jl#192 is merged. Thanks a lot!

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Feb 4, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Feb 4, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Mar 25, 2019

@lbenet

This comment has been minimized.

Copy link

lbenet commented Mar 25, 2019

@jedbrown We have just addressed your comments on the content. The pdf looks ok to me. I am waiting for the green lights of travis to merge to master. Is there something else to be done?

@lbenet

This comment has been minimized.

Copy link

lbenet commented Mar 25, 2019

@lbenet

This comment has been minimized.

Copy link

lbenet commented Mar 27, 2019

@whedon generate pdf

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Mar 27, 2019

Attempting PDF compilation. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Mar 27, 2019

@lbenet

This comment has been minimized.

Copy link

lbenet commented Mar 29, 2019

@jedbrown Is there something left to be done here?

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 2, 2019

@lbenet Thanks for your patience. (I was offline last week.) Please fix this citation JuliaDiff/TaylorSeries.jl@98281c2#r33009220
There is also a comment in figure 3 that spills into the margin. Can you shorten it? We should be ready to archive once these formatting issues are fixed.

@lbenet

This comment has been minimized.

Copy link

lbenet commented Apr 3, 2019

@whedon generate pdf from branch joss3

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 3, 2019

Attempting PDF compilation from custom branch joss3. Reticulating splines etc...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 3, 2019

@lbenet

This comment has been minimized.

Copy link

lbenet commented Apr 3, 2019

@jedbrown I just opened JuliaDiff/TaylorSeries.jl#203; the citation was fixed (link to Wikipedia) and the comment of Fig 3 shortened. I also added the tex files to generate the pdf's (I cropped the pdf's manually though).

@lbenet

This comment has been minimized.

Copy link

lbenet commented Apr 3, 2019

Let me know if I need to do something else before merging.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 4, 2019

Looks good to me, thanks. Please merge and tag (annotated tag preferred) and archive on Zenodo or similar, then report the DOI back here.

@lbenet

This comment has been minimized.

Copy link

lbenet commented Apr 4, 2019

Thanks a lot @jedbrown for all your work and comments.

I merged JuliaDiff/TaylorSeries.jl#203, just tagged a new minor version (0.9.2), which has been accepted in METADATA.jl (current central repo of registered packages, but not for long). (I hope I annotated it properly; sorry if not.) The DOI at zenodo (v0.9.2) is: 10.5281/zenodo.2628898.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 5, 2019

@whedon set v0.9.2 as version

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 5, 2019

OK. v0.9.2 is the version.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 5, 2019

@whedon set 10.5281/zenodo.2628898 as archive

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 5, 2019

OK. 10.5281/zenodo.2628898 is the archive.

@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 5, 2019

@whedon accept

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 5, 2019

Attempting dry run of processing paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 5, 2019


OK DOIs

- 10.5281/zenodo.2557003 is OK
- 10.1137/141000671 is OK
- 10.1007/978-3-319-29662-3 is OK
- 10.1145/1236463.1236468 is OK
- 10.5281/zenodo.2562364 is OK

MISSING DOIs

- None

INVALID DOIs

- None
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 5, 2019

Check final proof 👉 openjournals/joss-papers#607

If the paper PDF and Crossref deposit XML look good in openjournals/joss-papers#607, then you can now move forward with accepting the submission by compiling again with the flag deposit=true e.g.

@whedon accept deposit=true
@jedbrown

This comment has been minimized.

Copy link
Member

jedbrown commented Apr 5, 2019

@openjournals/joss-eics Over to you, please.

@kyleniemeyer

This comment has been minimized.

Copy link
Collaborator

kyleniemeyer commented Apr 7, 2019

@whedon accept deposit=true

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 7, 2019

Doing it live! Attempting automated processing of paper acceptance...
@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 7, 2019

🚨🚨🚨 THIS IS NOT A DRILL, YOU HAVE JUST ACCEPTED A PAPER INTO JOSS! 🚨🚨🚨

Here's what you must now do:

  1. Check final PDF and Crossref metadata that was deposited 👉 openjournals/joss-papers#608
  2. Wait a couple of minutes to verify that the paper DOI resolves https://doi.org/10.21105/joss.01043
  3. If everything looks good, then close this review issue.
  4. Party like you just published a paper! 🎉🌈🦄💃👻🤘

Any issues? notify your editorial technical team...

@kyleniemeyer

This comment has been minimized.

Copy link
Collaborator

kyleniemeyer commented Apr 7, 2019

Congrats @lbenet on your article's publication in JOSS!

Thanks to @sriharikrishna and @tobydriscoll for reviewing, and @jedbrown for editing.

@whedon

This comment has been minimized.

Copy link
Collaborator Author

whedon commented Apr 7, 2019

🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉

If you would like to include a link to your paper from your README use the following code snippets:

Markdown:
[![DOI](http://joss.theoj.org/papers/10.21105/joss.01043/status.svg)](https://doi.org/10.21105/joss.01043)

HTML:
<a style="border-width:0" href="https://doi.org/10.21105/joss.01043">
  <img src="http://joss.theoj.org/papers/10.21105/joss.01043/status.svg" alt="DOI badge" >
</a>

reStructuredText:
.. image:: http://joss.theoj.org/papers/10.21105/joss.01043/status.svg
   :target: https://doi.org/10.21105/joss.01043

This is how it will look in your documentation:

DOI

We need your help!

Journal of Open Source Software is a community-run journal and relies upon volunteer effort. If you'd like to support us please consider doing either one (or both) of the the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.