Joss-reviews: [REVIEW]: rowan: A Python package for working with quaternions

[whedon]

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:

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

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:

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 @labarba know.

Review checklist for @pdebuyl

Conflict of interest

• [x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• [x] Version: Does the release version given match the GitHub release (v1.0.0)?
• [x] Authorship: Has the submitting author (@vyasr) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [x] 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

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• [x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• [x] 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

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

Review checklist for @CorySimon

Conflict of interest

• [x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

• [x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

• [x] Repository: Is the source code for this software available at the repository url?
• [x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
• [x] Version: Does the release version given match the GitHub release (v1.0.0)?
• [x] Authorship: Has the submitting author (@vyasr) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [x] 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

• [x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• [x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• [x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• [x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
• [x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• [x] 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

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

[whedon]

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 :tada:.

:star: Important :star:

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:

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

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

@whedon commands

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[labarba]

@pdebuyl, @CorySimon -- here is where the magic happens. Let me know if you have any questions.

[pdebuyl]

Hi,

The rowan project gives a very good impression! The source code is well structured, the
design of the API is direct to understand and fully documented.

I did tick most of the checklist already. I have a few improvements to request:

• In the documentation file doc/index.rst, there should be a blank line below the
  code-block directives else sphinx complains (ref:
  http://www.sphinx-doc.org/en/stable/rest.html#directives)

• The "issues" page on Bitbucket is not accessible. It is however recommended to use it in the
  documentation.

• The license has Python highligthing. Add :language: none as a directive option to
  literalinclude.

• The definitions of the derivative and integral in rowan.calculus are not given. Please
  mention the definitions in the docstrings.

• In comparison to the README, the sphinx documentation misses the installation and
  quickstart instructions. If you add them, the sphinx documentation will be complete.

• The installation instruction for the documentation should also install
  'sphinx_rtd_theme'. This theme is no longer installed by default.

• The package claims to be efficient. The design makes it very probable but you should
  quantify this and mention whether all the algorithms operate at the broadcasting level or
  if some are left aside.

• As per the JOSS reviewer guidelines, it is important to cite prior
  software. http://matthew-brett.github.io/transforms3d/ should also be cited then.

[vyasr]

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).

• [x] Fix spacing for code-blocks
• [x] Make issues page publicly accessible
• [x] Add language directive to License
• [x] Add definitions of derivative and integral to rowan.calculus
• [x] Add installation and quickstart instructions to the sphinx documentation doc/index.rst
• [x] Add sphinx_rtd_theme to installation instructions in docs and README
• [x] Provide some form of benchmarks
• [x] Cite transforms3d as well

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[labarba]

Regarding performance claims, or comparison with other software, you can include this in the documentation, with a brief summary of results on the paper.

[pdebuyl]

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.

[vyasr]

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.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[pdebuyl]

@vyasr Yes, archiving on completion of the review is fine. Thank you for all the updates in response to the review :-)

@labarba The submission is 100% ok for me.

[vyasr]

Great thanks for the review @pdebuyl !

[vyasr]

@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.

[labarba]

Yes, @vyasr, hang tight. The second reviewer will start soon: I know he had commitments running up to this weekend.

[vyasr]

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.

[CorySimon]

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.

1. Upon python -m unittest discover tests, I get the following warning; is it significant?

............................./usr/lib/python3.5/importlib/_bootstrap.py:222: RuntimeWarning: numpy.dtype size changed, may indicate binary incompatibility. Expected 96, got 88
  return f(*args, **kwds)
/usr/lib/python3.5/importlib/_bootstrap.py:222: RuntimeWarning: numpy.dtype size changed, may indicate binary incompatibility. Expected 96, got 88
  return f(*args, **kwds)
..................................
----------------------------------------------------------------------
Ran 63 tests in 0.630s

OK

1. Mention in the docs where to run the tests, i.e. "cd into the test directory of the repository"

2. "Quaternions form a number system with various interesting properties, and they have a number of uses." seems a bit vacuous, maybe be more concrete and mention in the documentation the areas where quaternions are most widely used.

3. In the "random" section of the docs, instead of "Generate random rotations uniformly", I would spell out "uniformly distributed random rotations on a unit sphere" to be more precise.

4. When I run this example from the docs,

import rowan
import numpy as np

rands = np.random.rand(100, 3)
alpha, beta, gamma = rands.T
ql = rowan.from_euler(alpha, beta, gamma)
alpha_return, beta_return, gamma_return = rowan.to_euler(ql)
assert(np.all(alpha_return == alpha))
assert(np.all(beta_return == beta))
assert(np.all(gamma_return == gamma))

I get an error ValueError: too many values to unpack (expected 3)

1. Some of the examples in the documentation are intended to be self-contained, in that I can open up Python for the first time and copy-and-paste the examples in to run them because they contain the necessary import's (e.g. rotate and reflect, except you still need to from rowan import reflect). Some are not, e.g. by not including import rowan or from rowan import * or from rowan.calculus import integrate. Would that be better to put the necessary imports in the example so they are self-contained, or no? At least, I would be consistent.

2. Could you provide examples for the functions in rowan.random in the docs?

3. Arguably, the most complicated functions are under mapping. Copying the scikit-learn model, I think it would be helpful if you could provide a toy example of their use: two sets of points in 2D that need aligned, for example. You can generate the toy data set with a known shift and transformation matrix, then show that your functions recover the result and show the plots before and after alignment. It would be powerful and also serve as a nice displayed test that the more complicated functions are working. I do see you have something like this in rowan/tests/test_mapping.py, I'm suggesting you use the example to document the mapping module.

4. I suggest putting the performance bar plot (you can condense it by putting three bars next to each other per algorithm) in the documentation. This will show users in what cases they should choose your package over others, and also let other contributors know where to focus their efforts. Please put an xlabel on the performance plot as well.

[vyasr]

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.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

Hey @CorySimon let me know if I've sufficiently addressed your comments.

1. This issue has to do with importing scipy or scikit-learn that was compiled against an older version of numpy. Since scipy (and other packages) are typically built against the oldest supported version of numpy, I don’t think it’s something that I need to worry about.
2. I added a note that tests can be run from the root of the repository.
3. I updated my index.rst file with a bit of a better description.
4. Fixed.
5. Good catch, thanks. This example was out of date from an older version of the API, and it has now been fixed.
6. Yes, this is a good point. I also realized that I didn’t have examples in some of the subpackages, so I added those as well. I have now standardized this: any non-rowan imports (namely numpy) that are needed for a particular example are included, but imports of rowan itself are not included since I think that’s implied by every example. Additionally, I standardized the assumption across all examples that we are just importing rowan, not doing a from rowan import * or something else, so all functions are called by completely resolving them rather than doing, e.g., from rowan import random; random.rand(1).
7. Done (also discussed in point 6)
8. Done, this also came up regarding point 6. The examples are mostly the same for all the functions except ICP, which is a bit modified.
9. Done

[labarba]

@CorySimon --- Do let me know if you are satisfied with the author's mods, and if you are ready to recommend acceptance of this submission. Thanks for your contribution to JOSS!

[CorySimon]

Yes, thank you @vyasr for taking my suggestions into account. Rowan is a nice piece of software, and I'm sure many will find it useful!

I apologize for taking 13 days to see this; I don't recall seeing an email notification from the above post, and this Github account is linked to my personal email.

As an intriguing side note, I also like the story of how quaternions were discovered/invented. Perhaps a visit to the Brougham Bridge is in order for celebration of v1.0 of rowan!

[labarba]

Great, sounds like both reviewers are satisfied.

@vyasr, now, I do find that the figure is formatted poorly: font sizes show much too small. Could you make them larger and simply stack them on top of each other. You do have a third page with ample vertical white space, so moving content to the third page will be fine.

The sentence _"This package arose due to the representation…"_ is a bit awkward. How can "the representation" be a cause? Maybe "due to the need to represent"?

_"The resulting code fragmentation makes… and fails "_ --- I think you can use the past tense here: _made_ and _failed_ (since you now have rowan).

_"The package was incorporated into ... as well some"_ --- as well _as_ some.

Last sentence: "going forward" appears twice.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[labarba]

Good effort! Were you able at any point to get the figures of a smaller size, so a short text snippet would fit under them in page 2?

[vyasr]

That's what I'd like to do! I'm having a bit of trouble with it though (sorry for the deluge of recompilations). Can I control the figure size directly in any way? It's currently a bit larger than I'd like, so not only does no text fit under it, but also the labels on the left are getting cut off a little.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[labarba]

I agree that they are a bit large. I'm not sure if we can have embedded HTML, in which case you could use the width property:

img src="dir/file.png" width="200">

[labarba]

How about just moving the figures to the end? Not ideal, but might work better in this case.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

I gave the HTML solution a shot, let's see how it looks. If it doesn't work, then I'll do as you suggest and move the figure to the end.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

If I split the paragraph up I can get some of the text onto the first page, but the figure insists on taking up the full page and being a bit too wide to properly fit the existing margins.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[labarba]

Yikes. They don't appear if you place them in the end???

(I always tell my students: prepare your figures and output the image in the size you will use in the document :-)

@arfon Help! We're having problem with float placement!

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

Ooops no that was a different problem, a different way I tried to fix the size. Clearly did not work too well!

Yes, perhaps it will work better if I take the programatically generated png, import it into a properly sized Illustrator artboard, and then export that as PDF. Currently no matter what size I output the png as, it's getting resized to the full width of the page.

[labarba]

Then you could add white side margins _to the figure_, leaving the vertical image size the same.

[labarba]

It's currently not so bad (and fits in 3 pages), but the figure caption clashes with the footer rule.

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

OK the width issue appears to be fixed now. The primary outstanding problem is the figure height. Additional padding to the figure just leaves additional white space between the figure content and the caption, but the caption position still overlaps the footer rule.

[arfon]

@vyasr - you can limit the figure height like this:

![Performance Comparison](Performance.pdf){ height=750px }

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

@whedon generate pdf

[whedon]

Attempting PDF compilation. Reticulating splines etc...

[whedon]

--> Check article proof :page_facing_up: <--

[vyasr]

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.

[labarba]

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!

[pdebuyl]

@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 https://github.com/openjournals/joss/issues/400 to be fixed.

[arfon]

@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.

[vyasr]

@arfon Here's the doi: 10.5281/zenodo.1323676.

Zenodo link

[labarba]

@pdebuyl You didn't tick the version item on the checklist ... I assume this is fine?

[pdebuyl]

Yes, it is.

[arfon]

@whedon set 10.5281/zenodo.1323676 as archive

[arfon]

@whedon commands

[whedon]

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

[arfon]

@whedon set 10.5281/zenodo.1323676 as archive

[whedon]

OK. 10.5281/zenodo.1323676 is the archive.

[arfon]

@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 :zap: :rocket: :boom:

[whedon]

:tada::tada::tada: Congratulations on your paper acceptance! :tada::tada::tada:

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.00787/status.svg)](https://doi.org/10.21105/joss.00787)

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

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:

• Volunteering to review for us sometime in the future. You can add your name to the reviewer list here: http://joss.theoj.org/reviewer-signup.html
• Making a small donation to support our running costs here: https://www.flipcause.com/secure/cause_pdetails/Mjk3Nzk=

[vyasr]

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.

[labarba]

All share!
https://twitter.com/JOSS_TheOJ/status/1024063365482725376