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]: rowan: A Python package for working with quaternions #787
Comments
Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @pdebuyl, 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:
For a list of things I can do to help you, just type:
|
|
@pdebuyl, @CorySimon -- here is where the magic happens. Let me know if you have any questions. |
Hi, The rowan project gives a very good impression! The source code is well structured, the I did tick most of the checklist already. I have a few improvements to request:
|
Thanks for the comments. I'll keep a running checklist here as I address them. Regarding the efficiency, how would you recommend I do this? For example, are you interested in O(N) scaling? I can also include explicit benchmarks against the other code bases, I've done something like this before so I could formalize that script and include it in the repository as well, or just discuss it in the paper (with a figure or so).
|
@whedon generate pdf |
|
Regarding performance claims, or comparison with other software, you can include this in the documentation, with a brief summary of results on the paper. |
Hi @vyasr thank you for the update. The solution you propose to time the other packages is fine. You can keep the mention in the paper brief of course. All that will be missing then is the archived version of the software, for which you will probably wait for the other review. If you need assistance for archiving software on Zenodo, you can request it in this review page. |
I've updated the paper with a couple of sentences about performance, and I've added a benchmarks folder with a Jupyter notebook that compares to a pair of alternative solutions. I've noted the existence of these benchmarks in the documentation as well. Regarding the archived version, I was under the impression that the Zenodo archiving happened once the paper was published. Correct me if I'm wrong though. |
@whedon generate pdf |
|
Great thanks for the review @pdebuyl ! |
@labarba at this point I don't need to do anything else until the other reviewer begins his review, correct? Just want to make sure I'm not holding up the process. |
Yes, @vyasr, hang tight. The second reviewer will start soon: I know he had commitments running up to this weekend. |
No problem, as long as I'm not expected to do anything to alert him that it's ready. Whenever the review commences is fine with me. |
hi @vyasr, Very nice, professional, and well-documented Python package! Coincidentally, I recently implemented a rotation Monte Carlo move for molecules in my code and was looking into quaternions, but went with the method in "Fast Random Rotation Matrices" here. I'm curious about the ad-/dis-advantages of using quaternions over this. My suggestions/comments, some more trivial than others, are listed below.
I get an error
|
Thanks for the comments! I'm currently at SciPy, so I don't have too much bandwidth this week, but I will respond as soon as possible. |
@whedon generate pdf |
|
Hey @CorySimon let me know if I've sufficiently addressed your comments.
|
@whedon generate pdf |
|
@whedon generate pdf |
|
@whedon generate pdf |
|
Thanks @arfon, that worked! @labarba I think it's fine now. I tried playing with it a bit to see if I could get some text onto the same page as the figure, but the figure needs to be quite small before that happens so I did just enough to fix the issues with the footer. Let me know if you would like me to try reflowing text around the figure somehow though. |
Looks good now, phew! @arfon — this submission is ready to process for publication. @pdebuyl, @CorySimon — Thank you for your contribution to this adventure in new scholarly publishing! |
@labarba and all, my pleasure :-) BTW, if you wish to fix the author naming issue in the reference (that was mentioned in the pre-review), you will need to wait for openjournals/joss#400 to be fixed. |
@vyasr - At this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission. |
@arfon Here's the doi: 10.5281/zenodo.1323676. |
@pdebuyl You didn't tick the version item on the checklist ... I assume this is fine? |
Yes, it is. |
@whedon set 10.5281/zenodo.1323676 as archive |
@whedon commands |
Here are some things you can ask me to do:
|
@whedon set 10.5281/zenodo.1323676 as archive |
OK. 10.5281/zenodo.1323676 is the archive. |
@pdebuyl, @CorySimon - many thanks for your reviews here and to @labarba for editing this submission ✨ @vyasr - your paper is now accepted into JOSS and your DOI is https://doi.org/10.21105/joss.00787 ⚡ 🚀 💥 |
🎉🎉🎉 Congratulations on your paper acceptance! 🎉🎉🎉 If you would like to include a link to your paper from your README use the following code snippets:
This is how it will look in your documentation: 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:
|
Thanks @pdebuyl @CorySimon for your reviews! And thank you @labarba @arfon for managing the process. @CorySimon unrelated, I just realized that in your original review you asked a question about quaternions vs. rotation matrices. In general, the main advantage of quaternions is that the actual rotation operation is much faster. Even though the raw multiplication requires more operations, quaternions are faster to normalize and also much cheaper to chain (if you need to apply multiple rotations). There are good algorithms for both for generating random rotations, but it's the actual rotation operations that are costly. Also, for certain applications (such as on-board computers) where memory is a factor, quaternions use substantially less memory (4 numbers instead of 9). This is also why Euler angles still get used despite their flaws (only 3 numbers). A final advantage of quaternions is that you can interpolate between them, although that's not so relevant outside of graphics applications. |
Submitting author: @vyasr (Vyas Ramasubramani)
Repository: https://bitbucket.org/glotzer/rowan
Version: v1.0.0
Editor: @labarba
Reviewer: @pdebuyl, @CorySimon
Archive: 10.5281/zenodo.1323676
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
@pdebuyl & @CorySimon, 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 @labarba know.
Review checklist for @pdebuyl
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 @CorySimon
Conflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
paper.md
file include a list of authors with their affiliations?The text was updated successfully, but these errors were encountered: