search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
703 stars 36 forks source link

[REVIEW]: Larq: An Open-Source Library for Training Binarized Neural Networks #1746

Closed whedon closed 4 years ago

whedon commented 4 years ago

Submitting author: @lgeiger (Lukas Geiger) Repository: https://github.com/larq/larq Version: v0.8.3 Editor: @terrytangyuan Reviewer: @sbrugman, @casperdcl, @EduPH Archive: 10.6084/m9.figshare.11619912.v1

Status

status

Status badge code:

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

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) by leaving comments 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

@sbrugman & @casperdcl & @EduPH, 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.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @terrytangyuan know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @sbrugman

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @casperdcl

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @EduPH

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @sbrugman, @casperdcl, @EduPH it looks like you're currently assigned to review 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:

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

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

lgeiger commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

sbrugman commented 4 years ago

Hi @lgeiger, thanks for your submission.

I'm doing this review in parts, as for today I don't have time to fully read up on the topic, but I wanted to provide some feedback for you to be able to start. This comment contains those feedback points. For now, I took the documentation and code into consideration, not the paper.

The Larq ecosystem seems to be an excellent contribution to the open-source community and supports a thrilling research direction. In general, I'm positive. The comments below are intended to help improve the clarity, consideration and completeness of the contribution.

Documentation

A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?

The documentation clearly states that it is designed to solve power consumption and speed issues with deep neural network architectures. A notion of the target audience is mentioned on the 'about' page ("researchers and developers"), 'papers using Larq' page and in the last paragraph of the paper. I suggest clarifying this directly on the homepage.

The documentation and paper make no claims on the comparison of performance between Quantized Neural Networks and their high-precision weight and bias counterparts. At the time of writing, there is clearly a trade-off (although the authors are working hard to close the gap). I suggest being transparent in this difference and possibly opening the option of educating users on when a few percents of accuracy on a specific dataset are essential and when not. This might be especially relevant for people working in the industry.

Community: 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

There is a community menu item with a contribution guide tab in the documentation. This clearly states the steps for third parties wishing to contribute to the software (point 1). The other use cases are undocumented. Links to GitHub issues and spectrum are missing in the document.

Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).

The authors provide numerous examples. There are even hosted examples. In general, the examples are well-documented, except for the Larq Zoo. There is minimal context at these examples, which are more like code snippets. I suggest extending the description for these examples somewhat.

Other points

Feel free to take over any of the suggestions. I will complete the review at a later time.

casperdcl commented 4 years ago

Thanks @lgeiger. More feedback:

General checks

Functionality

def __init__(
    self,
    units,
    activation=None,
    use_bias=True,
    input_quantizer=None,
    kernel_quantizer=None,
    kernel_initializer="glorot_uniform",
    bias_initializer="zeros",
    kernel_regularizer=None,
    bias_regularizer=None,
    activity_regularizer=None,
    kernel_constraint=None,
    bias_constraint=None,
    metrics=None,
    **kwargs,
):
    super().__init__(
        units,
        activation=activation,
        use_bias=use_bias,
        input_quantizer=input_quantizer,
        kernel_quantizer=kernel_quantizer,
        kernel_initializer=kernel_initializer,
        bias_initializer=bias_initializer,
        kernel_regularizer=kernel_regularizer,
        bias_regularizer=bias_regularizer,
        activity_regularizer=activity_regularizer,
        kernel_constraint=kernel_constraint,
        bias_constraint=bias_constraint,
        metrics=metrics,
        **kwargs,
    )

Documentation

Software paper

EduPH commented 4 years ago

Hi! Here I provide a preliminary review. The library seems interesting and useful, and the documentation is appealing. I suggest the following considerations:

Functionality

  1. Installation was as described in the documentation.
  2. I could reproduce the examples. However, kernel died using Tensorflow 1.13 (I had to upgrade to version 2.0.0a0.

Documentation

  1. The documentation lacks formality (in a mathematical sense). I miss a proper definition of key concepts such as "quantizer" and others.
  2. In the Home section the first example of a model is shown. I think it should be described with more details.
  3. There is a LaTex compilation problem in "\frac{\partial L}{\partial w}".

Software paper

It is too short. I suggest to add:

  1. A brief introduction to BNN.
  2. A formal description of key concepts.
  3. Performance results (if possible, compared to non-BNN implementations to show its usefulness).
sbrugman commented 4 years ago

Hi @lgeiger,

We can see great progress at larq. Keep it up.

This round of feedback complements the earlier comments.

Software paper

State of the field: Do the authors describe how this software compares to other commonly-used packages?

Good luck. I am looking forward to the next revision of the draft.

terrytangyuan commented 4 years ago

Regarding the authorship, I think we would need explicit author names. If this is not possible to obtain, I’d be curious to see why it’s so.

lgeiger commented 4 years ago

Thank you very much about the thorough review and the PRs to larq. Your feedback is extremely useful for us.

I am also doing the reponse in parts. For now I tried to address all the issues related to the library itself and the documentation. Please let me know if I am missing something.

Response to comments from @sbrugman

The documentation clearly states that it is designed to solve power consumption and speed issues with deep neural network architectures. A notion of the target audience is mentioned on the 'about' page ("researchers and developers"), 'papers using Larq' page and in the last paragraph of the paper. I suggest clarifying this directly on the homepage.

I tried to clarify it in https://github.com/larq/larq/pull/261

The documentation and paper make no claims on the comparison of performance between Quantized Neural Networks and their high-precision weight and bias counterparts. At the time of writing, there is clearly a trade-off (although the authors are working hard to close the gap). I suggest being transparent in this difference and possibly opening the option of educating users on when a few percents of accuracy on a specific dataset are essential and when not. This might be especially relevant for people working in the industry.

I agree. I made an issue to track our progress on this: https://github.com/larq/larq/issues/262

There is a community menu item with a contribution guide tab in the documentation. This clearly states the steps for third parties wishing to contribute to the software (point 1). The other use cases are undocumented. Links to GitHub issues and spectrum are missing in the document

Added in https://github.com/larq/larq/pull/237

The authors provide numerous examples. There are even hosted examples. In general, the examples are well-documented, except for the Larq Zoo. There is minimal context at these examples, which are more like code snippets. I suggest extending the description for these examples somewhat.

Fixed in https://github.com/larq/larq/pull/235

For each of the items in the API, one could ask: how does this compare to the underlying framework and why does it need an extension? Most of the sections (e.g. Quantizers) answer these questions, for example by providing references. An example where this information is missing is sign. Adding a reference to the tensorflow sign and/or mentioning that tf.math.sign would be a quick win.

Fixed in https://github.com/larq/larq/pull/238

The FAQ is now located under "Why Larq?", which I found to be an illogical location.

Fixed in https://github.com/larq/larq/pull/239

Inline math is not rendered (ex. key concepts).

Fixed in https://github.com/larq/larq/pull/228

Consider adding references to the home page (e.g. Courbariaux).

Fixed in https://github.com/larq/larq/pull/258

Suggestions for spelling and grammar can be found here.

https://github.com/larq/larq/pull/227 Thanks for the fixes :+1:

Response to comments from @casperdcl

Installation instructions: Installation (https://github.com/larq/larq#installation and https://larq.dev/#installation) also needs to also document pip install tensorflow, pip install tensorflow-gpu (or add them to setup.py etc), referencing the earlier link (https://www.tensorflow.org/install) for additional dependencies.

Unfortunately we can't add tensorflow as a direct dependency because that would break usage with GPU. We clarified the instructions: https://github.com/larq/larq/pull/240

Performance: There is frequent mention of the efficiency of BNNs, however it would be nice to have e.g. a quantitative table of results showing that larq matches theoretical/literature speed improvements.

I fully agree, that this would be really useful. However, Larq is currently only focused on research and training extremely quantized networks. That means there is currently no deployment workflow available and no optimized kernels implemented. Quoting theoretical numbers is always tricky since it really depends on the platform and runtime the models are deployed. That being said, we are working on a compute engine for Larq that will allow optimized deployment using TFLite on ARM and mobile platforms at which point we will publish real world numbers and benchmarks as soon as possible.

Submission requirements: I'm not sure but as per https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements the software seems to be bordering on a "'thin' API client" around tensorflow.keras. Perhaps it's worth submitting to JOSE instead? Or including in a larger publication similar to the Helwegen et al. paper? For example ls larq/.py larq/snapshots/.py | grep -v test | xargs cloc yields 1,586 non-blank/comment lines, with the main layers.py file exclusively made up of lines which look like.

It is true that the API closely mirrors tf.keras. However, this was a deliberate design choice early on in order to be compatible with the entire ecosystem and not introduce a new abstraction in order to be easy to adopt for existing Keras users. We explored some different design ideas early on in https://github.com/larq/larq/issues/4 and concluded that the current API design is probably the most ergonomic for users. larq/layers.py is purely for documentation purpose. The (also pretty straight forward) actual implementations are located under larq/layers_base.py. While Larq layers only add a few keyword argument to compared to tf.keras layers, under the hood they implement functionality that isn't available otherwise without completely reimplementing the custom layers from scratch. In addition to the layers we have added metrics and quantizers specific to binarized and other extremely quantized neural networks which are too specific to upstream into tf.keras itself.

A statement of need: https://larq.dev/about/ doesn't mention students (while the paper does).

Fixed in https://github.com/larq/larq/pull/241

Functionality documentation: The API link (https://larq.dev/api/layers/) seems to imply this is just larq.layers, yet the Models link (https://larq.dev/models/) gives a separate page which seems to be detailing API for larq/zoo which seems to be a separate package built on top of the larq.layers API. If this is the case it needs to be made clearer.

We tried to make the distinction clearer: https://github.com/larq/larq/pull/242

Response to comments from @EduPH

The documentation lacks formality (in a mathematical sense). I miss a proper definition of key concepts such as "quantizer" and others.

We intentionally opted for a less formal language to reduce jargon and make the documentation a bit more approachable for people without a math background. However I see your point that our explanation is unclear from a mathematical view. I tried to add a small mathematical clarification in https://github.com/larq/larq/pull/259

In the Home section the first example of a model is shown. I think it should be described with more details.

I extended the description and added a link to the more detailed user guide: https://github.com/larq/larq/pull/261

There is a LaTex compilation problem in "\frac{\partial L}{\partial w}".

Fixed in https://github.com/larq/larq/pull/228

lgeiger commented 4 years ago

We need to discuss what we think of "Plumerai team" as an author, as anonymous authorship is unconventional. @terrytangyuan, do you know of any precedent of this at the JOSS?

Thanks for bringing this up. We also aren't sure about how to deal with this. Currently all main contributors of Larq are part of the Plumerai Research Team. Some contributions (initial API design and feedback, work on planned improvements, etc) don't show up on GitHub. Therefor we opted for a group authorship to keep it timeless and also give credit to people who are currently working on larq but are not listed by GitHub. This way we tried to avoid making a distinction between major and minor contributions (I think every contribution is valuable) and we would also avoid missing authors that will join the team in the future. Nevertheless I am happy to update the author list to include individual people. @terrytangyuan Does JOSS have a policy or examples about projects that are not lead by an individual person, but by a group of people or an entire research lab?

terrytangyuan commented 4 years ago

@openjournals/joss-eics Could you advise on the question above? Thanks.

lgeiger commented 4 years ago

You could demonstrate this by for example going through the steps to implement a recent paper in larq (this paper might be a good candidate, source code in appendix). This point is critical as without research clear application, the added value is primarily educational.

@sbrugman I agree, the paper would be interesting to reproduce, though I am not sure if I'll find time for it any time soon. That being said, we already have multiple examples of how you would use Larq to reproduce a paper. All models currently available under https://larq.dev/models/ are reproductions of papers in the field of extremely quantized neural networks. In total we reproduced the results of 5 papers with tested implementations and fully reproducible training procedures available at https://github.com/larq/zoo/. (Unfortunately this process was sometimes quite frustrating since not all papers mentioned all the tricks used and often times only had incomplete or non functional code available 😞). I think this is definitely a useful resource when trying to do research on top of existing work. I'll try and stress that in the paper to make it clearer. Do you think we need to highlight this fact more in the documentation too?

kyleniemeyer commented 4 years ago

@lgeiger @terrytangyuan regarding the team authorship question, we've definitely seen this used by some software projects. From my/our perspective, if this is how the project prefers to be cited, then it's fine. (Or, if they don't make a list of authors/contributors obvious.)

kthyng commented 4 years ago

Hi @lgeiger, @sbrugman, @casperdcl, @EduPH: How is this review coming along? Looks like there has been a bunch of great work but it has stalled for the past few weeks.

lgeiger commented 4 years ago

Thanks for checking back. I will submit a second draft in the next few days. Sorry for the delay.

labarba commented 4 years ago

@lgeiger — Could we have a status update of your work on responding to reviewer comments? Let me know if you want us to set a reminder for you.

lgeiger commented 4 years ago

Thank you very much for all the reviews and the patience. I updated the paper to include a lot more background information and a more detailed description of the project. I am very much looking forward to your feedback.

lgeiger commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

terrytangyuan commented 4 years ago

@sbrugman @EduPH @casperdcl Could you take another look at the updated paper when you get a chance?

EduPH commented 4 years ago

sure!

EduPH commented 4 years ago

The API and the paper have improved a lot since the beginning of the review. Just some typos or minor corrections in the paper:

real valued -> real-valued networks in industry -> networks in the industry facilitate research to resolve -> facilitate researchers to resolve libaries -> libraries commmon -> common though in absence of a commmon API> though, in absence of a common API

lgeiger commented 4 years ago

Just some typos or minor corrections in the paper

Thanks a lot for the corrections, I updated the paper :+1:

lgeiger commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

sbrugman commented 4 years ago

Hi @lgeiger,

The paper has really benefitted from the revisions, good job. I have some suggestions:

lgeiger commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

lgeiger commented 4 years ago

Thanks for the comments, I tired to update the references to refer to the conference versions.

the same statement could hold for running the models in a data center

I don't think that is entirely true. The examples of self-driving cars or autonomous drones are not really possible with data center based networks due to latency or privacy constraints.

sbrugman commented 4 years ago

Let me clarify. The introduction appears unclear to me, when reading it for the first time. Most of the information you want to convey is there (you might want to add the latency or privacy constaints that you mention), though the information ordering is confusing to me.

The paper starts off with a lot to process, in my opinion. The first sentence implies that deep learning currently happends inside data centers. Only in the following sentences, we get to understand that computation plays an important role. The reader has to go back and forth trough the paragraph to really understand the first sentence. My suggestion would be to order the information so that the "reading flow" is sequential.

If you compare this to the second paragraph, you might see the difference. The second paragraph does a really good job at guiding the reader through the challenges in binarization.

I see that both @EduPH and me checked all boxes. @casperdcl, Could you have another look?

casperdcl commented 4 years ago
koenhelwegen commented 4 years ago 
* (minor) not sure about authorship; it seems that if the larq team is fine with the current paper's author list then this should not be a problem - but I can't see any confirmation of this.

Hi, I can confirm the team is happy with the current authorship! As the team is growing and we expect more people to work on Larq in the future, we prefer acknowledging the whole team over some set of individuals.

lgeiger commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

lgeiger commented 4 years ago

Happy 2020 everyone 🎉

Let me clarify. The introduction appears unclear to me, when reading it for the first time.

@sbrugman Thanks for clarifying. I rewrote the introduction to make it less confusing.

the "statement of need" in the paper is titled "Encouraging reproducible research in larq/zoo"

@casperdcl That is a good point, I haven't though about that. I consolidated the spread out statement of need into its own section.

(major) performance claims such as "64 times speed-up" should 1. have a literature citation and 2. come with proof that larq at least somewhat delivers on this theoretical promise.

@casperdcl Thanks for catching this, indeed performance claims like this can be very misleading. The "64 times speed-up" stems from the n^2 logic gates per n bits in classic multiplier circuits. I agree, this claim is problematic and would need real world measurements to confirm. I changed it to Compared to an equivalent 8-bit quantized network BNNs require 8 times smaller memory size and 8 times fewer memory accesses,... which is cited from Hubara et al., 2016.

casperdcl commented 4 years ago

Fine for me; the only outstanding point is potentially submit to JOSE instead as @sbrugman and I mentioned (due to the "arguably thin API-wrapping" [sic] nature of the library and "limited evidence of added value beyond purely educational" [sic]).

terrytangyuan commented 4 years ago

@whedon check references

whedon commented 4 years ago 
Reference check summary:

OK DOIs

- 10.1038/323533a0 is OK

MISSING DOIs

- https://doi.org/10.1163/1574-9347_bnp_e612900 may be missing for title: Keras
- https://doi.org/10.1007/978-3-030-01267-0_44 may be missing for title: Bi-Real Net: Enhancing the Performance of 1-bit CNNs With Improved Representational Capability and Advanced Training Algorithm
- https://doi.org/10.1007/978-3-319-46493-0_32 may be missing for title: XNOR-Net: ImageNet Classification Using Binary Convolutional Neural Networks
- https://doi.org/10.1109/cvpr.2019.00506 may be missing for title: Binary Ensemble Neural Network: More Bits per Network or More Networks per Bit?
- https://doi.org/10.1109/cvpr.2019.00050 may be missing for title: Structured Binary Neural Networks for Accurate Image Classification and Semantic Segmentation

INVALID DOIs

- None
terrytangyuan commented 4 years ago

It looks like all items in the checklists are completed. Thanks everyone!

@lgeiger Could you fix the above missing DOIs?

lgeiger commented 4 years ago

Thanks for the great reviews everyone!

Could you fix the above missing DOIs?

@terrytangyuan I fixed the missing DOIs. Note that "Keras" doesn't have a DOI and the one linked by whedon is not correct.

lgeiger commented 4 years ago

@whedon generate pdf

lgeiger commented 4 years ago

@whedon check references

whedon commented 4 years ago

PDF failed to compile for issue #1746 with the following error:

Error reading bibliography ./paper.bib (line 156, column 5): unexpected "d" expecting space, ",", white space or "}" Error running filter pandoc-citeproc: Filter returned error status 1 Looks like we failed to compile the PDF