[REVIEW]: Bézier

[whedon]

Submitting author: @dhermes (Daniel Hermes) Repository: https://github.com/dhermes/bezier Version: 0.3.0 Editor: @arfon Reviewer: @tacaswell Archive: 10.5281/zenodo.838308

Status

Status badge code:

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

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 questions

Conflict of interest

• [x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

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?
• [ ] Version: Does the release version given match the GitHub release (0.3.0)?
• [x] Authorship: Has the submitting author (@dhermes) made major contributions to the software?

Functionality

• [x] Installation: Does installation proceed as outlined in the documentation?
• [x] Functionality: Have the functional claims of the software been confirmed?
• [ ] Performance: Have any performance claims of the software been confirmed?

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 for JOSS. @tacaswell 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 as reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all JOSS reviews 😿

To fix this do the following two things:

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

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

[tacaswell]

• There are 145 commits to master from the 0.3.0 tag, changes include dependency changes
• missing link to docs on the github page (ended up using google to find them)
• could the DEVELOPMENT.rst page be included in the html docs?
• a bit more narrative as to the applications of this package on the main page would be good

Can not get the functional tests to run following the directions (more-or-less with python36) which I assume is due to changes with absolute import

23:03 $ python -m pytest functional_tests/
======================================================================================================== test session starts =========================================================================================================
platform linux -- Python 3.6.1, pytest-3.0.6, py-1.4.32, pluggy-0.4.0
rootdir: /home/tcaswell/src/o/bezier, inifile: tox.ini
plugins: cov-2.4.0, xdist-1.15.0
collected 0 items / 4 errors 

=============================================================================================================== ERRORS ===============================================================================================================
_______________________________________________________________________________________ ERROR collecting functional_tests/test_curve_curve.py ________________________________________________________________________________________
ImportError while importing test module '/home/tcaswell/src/o/bezier/functional_tests/test_curve_curve.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
functional_tests/test_curve_curve.py:20: in <module>
    import runtime_utils
E   ModuleNotFoundError: No module named 'runtime_utils'
_______________________________________________________________________________________ ERROR collecting functional_tests/test_segment_box.py ________________________________________________________________________________________
ImportError while importing test module '/home/tcaswell/src/o/bezier/functional_tests/test_segment_box.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
functional_tests/test_segment_box.py:16: in <module>
    import runtime_utils
E   ModuleNotFoundError: No module named 'runtime_utils'
______________________________________________________________________________________ ERROR collecting functional_tests/test_surface_locate.py ______________________________________________________________________________________
ImportError while importing test module '/home/tcaswell/src/o/bezier/functional_tests/test_surface_locate.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
functional_tests/test_surface_locate.py:17: in <module>
    import runtime_utils
E   ModuleNotFoundError: No module named 'runtime_utils'
_____________________________________________________________________________________ ERROR collecting functional_tests/test_surface_surface.py ______________________________________________________________________________________
ImportError while importing test module '/home/tcaswell/src/o/bezier/functional_tests/test_surface_surface.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
functional_tests/test_surface_surface.py:21: in <module>
    import runtime_utils
E   ModuleNotFoundError: No module named 'runtime_utils'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 4 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
====================================================================================================== 4 error in 0.83 seconds =======================================================================================================
(dd36) ✘-2 ~/src/o/bezier [0.3.0|✚ 3] 

but by changing to functional tests and running python -m pytest got them to pass

Getting one failure on the unit tests

(dd36) ✘-1 ~/src/o/bezier [0.3.0|✚ 3] 
23:17 $ python -m pytest tests/
======================================================================================================== test session starts =========================================================================================================
platform linux -- Python 3.6.1, pytest-3.0.6, py-1.4.32, pluggy-0.4.0
rootdir: /home/tcaswell/src/o/bezier, inifile: tox.ini
plugins: cov-2.4.0, xdist-1.15.0
collected 345 items 

tests/test__base.py .......
tests/test__curve_helpers.py ........................
tests/test__helpers.py .......................
tests/test__intersection_helpers.py ..................................................................
tests/test__plot_helpers.py ..
tests/test__surface_helpers.py .........................................................................................................
tests/test_curve.py .......................................
tests/test_curved_polygon.py ..........
tests/test_surface.py .F...................................................................

============================================================================================================== FAILURES ==============================================================================================================
______________________________________________________________________________________________ TestSurface.test___repr__custom_triangle ______________________________________________________________________________________________
Traceback (most recent call last):
  File "/home/tcaswell/src/o/bezier/tests/test_surface.py", line 124, in test___repr__custom_triangle
    nodes = np.zeros((num_nodes, dimension))
TypeError: 'float' object cannot be interpreted as an integer
================================================================================================ 1 failed, 344 passed in 4.10 seconds ================================================================================================

Only got part way through this review due to reading about Barycentric coordinate system and learning some new (to me) math!

[dhermes]

There are 145 commits to master from the 0.3.0 tag, changes include dependency changes

Indeed. I have had issues with the Windows build since adding the Fortran speedups. I'd love to do a release but don't want to leave out Windows users.

missing link to docs on the github page (ended up using google to find them)

There is a badge "docs | latest" in the README. I'm open to suggestions on adding something else / where you'd like to see it added.

could the DEVELOPMENT.rst page be included in the html docs?

Definitely.

a bit more narrative as to the applications of this package on the main page would be good

• By main page, do you mean http://bezier.readthedocs.io/en/latest/ or the GitHub README?
• When you say "more narrative", do you mean "work with Bezier curves / surfaces / etc." or "here's what a Bezier curve / surface / etc. is and you might want one because "?

Can not get the functional tests to run following the directions (more-or-less with python36) which I assume is due to changes with absolute import

Yes I noticed that yesterday when I was updated the CI / testing environment. Right now they are running in Python 2.7 on CI, but I've filed an issue to fix and will get to it ASAP tomorrow.

Getting one failure on the unit tests

I can't reproduce locally or in CI. Can we discuss your environment a bit more?

Only got part way through this review due to reading about Barycentric coordinate system and learning some new (to me) math!

They can be really great! One particularly awesome feature of Bezier curves / the Bernstein basis is the convexity property. It allows for lots of nice optimizations. I'm happy to chat about the math if you'd like me to shed some light on things. (The bibliography is a bit long.) I'm certainly not an authority on the subject but there is plenty I do know.

[tacaswell]

RE: link to docs: :sheep: I was looking for a blue link that said 'documentation' and totally skipped over the badges!

RE: narrative: both the README and the RTD docs (but content should be very similar). On the "here's what a Bezier curve / surface / etc. is and you might want one because ..." to address the statement of need.

---

The test failure is in def test___repr__custom_triangle(self): and is raised from the the test code due to

(dd36) ✘-1 ~/src/o/bezier [0.3.0|✚ 3] 
08:48 $ ipython
Python 3.6.1 |Continuum Analytics, Inc.| (default, Mar 22 2017, 19:54:23) 
Type "copyright", "credits" or "license" for more information.

IPython 5.2.2 -- An enhanced Interactive Python.
?         -> Introduction and overview of IPython's features.
%quickref -> Quick reference.
help      -> Python's own help system.
object?   -> Details about 'object', use 'object??' for extra details.

In [1]: import numpy

In [2]: import numpy as np

In [3]: np.zeros((13.0, 7))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-f0a5700cdff1> in <module>()
----> 1 np.zeros((13.0, 7))

TypeError: 'float' object cannot be interpreted as an integer

In [4]: np.__version__
Out[4]: '1.12.1'

[dhermes]

@tacaswell You're seeing this failure at HEAD or at the 0.3.0 tag?

UPDATE: The 0.3.0 version is the source of the failure due to the change in division in Python 3:

degree = 4
dimension = 3
num_nodes = ((degree + 1) * (degree + 2)) / 2
nodes = np.zeros((num_nodes, dimension))

The HEAD version does not have this issue:

degree = 4
dimension = 3
num_nodes = ((degree + 1) * (degree + 2)) // 2
nodes = np.zeros((num_nodes, dimension), order='F')

[dhermes]

FYI @tacaswell I added the DEVELOPMENT doc to the HTML / Sphinx docs: http://bezier.readthedocs.io/en/latest/development.html

[dhermes]

@tacaswell Anything you are waiting on anything from me?

[dhermes]

@tacaswell I have added a Why Bezier? section and also done a 0.4.0 release.

AFAICT I have addressed all of the current feedback, LMK what I need do from here (e.g. if the 0.4.0 release has thrown a wrench into this process).

[arfon]

Friendly reminder on this @tacaswell 😁

[dhermes]

@tacaswell @arfon Anything I can do here?

[tacaswell]

All of my issues have been addressed.

I recommend this to be published!

Sorry for being so slow on my end.

[tacaswell]

There are far more references in the bibliography than cited from paper.md, does that get sorted out at somepoint in the next step?

[arfon]

Thanks @tacaswell.

There are far more references in the bibliography than cited from paper.md, does that get sorted out at somepoint in the next step?

@dhermes - all of the references need moving to the paper.bib file and citing directly. (You can read how to do that here)

In addition, could you please expand a little on the paper.md file? We (the JOSS editors) are in the process of adding more guidance for authors, including a better indication of expected paper length. Currently it's hard to get a picture of the functionality of the software, and also include a statement of need, in less than 250 words. Would you be able to put a little more detail in your submission please?

[dhermes]

could you please expand a little on the paper.md file

Will do!

Should I mention features the library doesn't have (that could be added at a later date)? Following a standard text / course syllabus I could outline the "toolbox" one would typically want when dealing with Bezier curves and how it compares to the feature set of the library.

---

@arfon IIUC, when @tacaswell said

There are far more references in the bibliography than cited from paper.md

he meant that paper.bib has more references (23):

$ cat paper.bib | grep -e '^@'
@article{Farouki1987,
@article{Sederberg1989,
@article{Sederberg1989-alg,
@article{Sederberg1990,
@article{Farouki1991,
@article{Dunavant1985,
@article{Farouki1988,
@article{Farouki1990,
@article{Farouki1996,
@article{Kim1998,
@article{Sederberg1986,
@inproceedings{Roth,
@misc{Roth2000,
@article{Farin1986,
@article{Sederberg1986-improp,
@article{Sederberg1984,
@article{Brown1971,
@article{Farouki2012,
@article{Bus2008,
@article{JonssonVavasis,
@techreport{Manocha:CSD-92-698,
@book{Farin2001,
@misc{SederbergNotes,
$ cat paper.bib | grep -e '^@' | wc -l
23

than are actually used in paper.md:

# References

- [Lecture notes][3]: Computer Aided Geometric Design by Thomas W.
  Sederberg of BYU
- [Curves and Surfaces for CAGD][4] by Gerald Farin

...

[3]: http://tom.cs.byu.edu/~557/text/cagd.pdf
[4]: https://www.amazon.com/Curves-Surfaces-CAGD-Fifth-Practical/dp/1558607374

---

Those references are @book{Farin2001, ...} and @misc{SederbergNotes, ...}, and I added them as citations in paper.md

[arfon]

Should I mention features the library doesn't have (that could be added at a later date)? Following a standard text / course syllabus I could outline the "toolbox" one would typically want when dealing with Bezier curves and how it compares to the feature set of the library.

I think the paper should only describe the current functionality at a high level rather than write about what the library might do one day.

Also, please note, for the citations, the section after the # References heading should be completely empty. Pandoc compiles the references you have cited in your paper.md from the paper.bib file into the final output. You can use this paper as an example.

[dhermes]

I just updated the summary. LMK what you think @arfon and @tacaswell?

The 3 paragraphs are:

• What are Bézier curves / surfaces
• Where (i.e. what fields) have use for them? (This is essentially equivalent to stating the "audience")
• What features are present for people who might use them

[arfon]

This looks good to me @dhermes.

[arfon]

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

[dhermes]

Sure thing: 10.5281/zenodo.838308 is for bezier==0.4.0 (10.5281/zenodo.838307 is the "catch-all" DOI for the project)

[arfon]

@whedon set 10.5281/zenodo.838308 as archive

[whedon]

OK. 10.5281/zenodo.838308 is the archive.

[arfon]

@tacaswell many thanks for your review here ✨

@dhermes - your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00267 ⚡️ 🚀 :boom:

[dhermes]

:tada: :boom: w00t! Thanks both of you!