openjournals / joss-reviews

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

[REVIEW]: CircuitGraph: A Python package for Boolean circuits #2646

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @jpsety (Joseph Sweeney) Repository: https://github.com/circuitgraph/circuitgraph Version: v0.0.3 Editor: @danielskatz Reviewer: @skadio, @prw99r, @r-a-hoggarth Archive: 10.5281/zenodo.4302741

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

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

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

@skadio & @prw99r & @r-a-hoggarth, 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 @danielskatz know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @skadio

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @prw99r

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @r-a-hoggarth

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. @skadio, @prw99r, @r-a-hoggarth it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

: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
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1007/978-3-319-94144-8_26 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 4 years ago

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

danielskatz commented 4 years ago

πŸ‘‹ @skadio, @prw99r, @r-a-hoggarth - Thanks again for agreeing to review. Please read the comments above before starting.

This is the review thread for the paper. All of our communications will happen here from now on.

All reviewers have checklists at the top of this thread with the JOSS requirements. As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines.

The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention openjournals/joss-reviews#2646 so that a link is created to this thread (and I can keep an eye on what is happening). Please also feel free to comment and ask questions on this thread. In my experience, it is better to post comments/questions/suggestions as you come across them instead of waiting until you've reviewed the entire package.

We aim for reviews to be completed within about 2-4 weeks. Please let me know if any of you require some more time. We can also use Whedon (our bot) to set automatic reminders if you know you'll be away for a known period of time.

Please feel free to ping me (@danielskatz) if you have any questions/concerns.

danielskatz commented 4 years ago

πŸ‘‹ @skadio, @prw99r, @r-a-hoggarth - I just wanted to check on how things are going for you on your reviews. Let me know if I can do anything to help.

danielskatz commented 4 years ago

πŸ‘‹ @skadio, @prw99r, @r-a-hoggarth - Just checking in on this again...

prw99r commented 4 years ago

Just getting into the JOSS way of doing things - learning as I go...

On Thu, Sep 24, 2020 at 5:48 PM Daniel S. Katz notifications@github.com wrote:

πŸ‘‹ @skadio https://github.com/skadio, @prw99r https://github.com/prw99r, @r-a-hoggarth https://github.com/r-a-hoggarth - Just checking in on this again...

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-698462163, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSTHIUHT5FBTO3XNL6TSHNZ6ZANCNFSM4Q74WFHQ .

prw99r commented 4 years ago

@whedon How to I edit my review checklist?

whedon commented 4 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@whedon commands
prw99r commented 4 years ago

@danielskatz How do I edit my review checklist?

danielskatz commented 4 years ago

@whedon re-invite @prw99r as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@prw99r please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

r-a-hoggarth commented 4 years ago

@whedon re-invite @r-a-hoggarth as reviewer

whedon commented 4 years ago

I'm sorry @r-a-hoggarth, I'm afraid I can't do that. That's something only editors are allowed to do.

danielskatz commented 4 years ago

@whedon re-invite @r-a-hoggarth as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@r-a-hoggarth please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

danielskatz commented 4 years ago

It's good to see @r-a-hoggarth making progress, but I now wonder about @prw99r (no boxes checked) and @skadio (no changes recently). How are things going?

prw99r commented 4 years ago

@whedon commands

whedon commented 4 years ago

Here are some things you can ask me to do:

# List Whedon's capabilities
@whedon commands

# List of editor GitHub usernames
@whedon list editors

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

EDITORIAL TASKS

# Compile the paper
@whedon generate pdf

# Compile the paper from alternative branch
@whedon generate pdf from branch custom-branch-name

# Ask Whedon to check the references for missing DOIs
@whedon check references

# Ask Whedon to check repository statistics for the submitted software
@whedon check repository
prw99r commented 4 years ago

Hi @danielskatz - how do I check off items on the checklist? I'm probably being really dumb, but how the heck do I do that? Thanks, Peter

prw99r commented 4 years ago

Hi Daniel, I've actually made some progress on looking at the code (download the repo etc, having a good look at the code etc) - if I can access the checklist (or there are some commands to do it) then I can look like I'm actually doing something! Cheers Peter

On Sun, Oct 4, 2020 at 7:37 PM Daniel S. Katz notifications@github.com wrote:

It's good to see @r-a-hoggarth https://github.com/r-a-hoggarth making progress, but I now wonder about @prw99r https://github.com/prw99r (no boxes checked) and @skadio https://github.com/skadio (no changes recently). How are things going?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-703297257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSVIQBIK6ZTGO7YCFZDSJC6GTANCNFSM4Q74WFHQ .

danielskatz commented 4 years ago

@whedon re-invite @prw99r as reviewer

whedon commented 4 years ago

The reviewer already has a pending invite.

@prw99r please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

danielskatz commented 4 years ago

πŸ‘‹ @prw99r - you have to accept the invitation. See the comment above this one.

prw99r commented 4 years ago

it says the invitation has expired?

On Tue, Oct 6, 2020 at 11:59 AM Daniel S. Katz notifications@github.com wrote:

πŸ‘‹ @prw99r https://github.com/prw99r - you have to accept the invitation. See the comment above this one.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-704192424, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSREKQTBAUORRSHRHXDSJL2CFANCNFSM4Q74WFHQ .

danielskatz commented 4 years ago

@whedon re-invite @prw99r as reviewer

whedon commented 4 years ago

OK, the reviewer has been re-invited.

@prw99r please accept the invite by clicking this link: https://github.com/openjournals/joss-reviews/invitations

danielskatz commented 4 years ago

It must have been the one from the previous time expired between when I sent it and now - the new one should be available for 10-11 days, I think... Please try again

prw99r commented 4 years ago

Got it - it works, I'm in!

On Tue, Oct 6, 2020 at 1:22 PM Daniel S. Katz notifications@github.com wrote:

It must have been the one from the previous time expired between when I sent it and now - the new one should be available for 10-11 days, I think... Please try again

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-704231430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSS557CUXHUGIWBXLL3SJMDW5ANCNFSM4Q74WFHQ .

danielskatz commented 4 years ago

πŸ‘‹ @skadio, @prw99r, @r-a-hoggarth - Can I again check in with you to see how things are going? I see a bunch of boxes checked, but some not checked. For each of you, are the unchecked boxes thing that you just haven't been able to check on yet, or are they items where there is work needed by the submitter, or discussion needed? If so, please say what these blocking issues are.

Otherwise, can you tell me when you think you will be done with your reviews?

danielskatz commented 4 years ago

πŸ‘‹ @skadio, @prw99r, @r-a-hoggarth - ping... ⬆️

danielskatz commented 4 years ago

@r-a-hoggarth - it looks like you've checked off everything - can I take that to mean you are satisfied with the submission?

skadio commented 4 years ago

A quick update: I looked at the submissions but haven't completed the list yet, hoping to get back to it this weekend.

prw99r commented 4 years ago

I have some time scheduled this week to check things out some more.

On Thu, 29 Oct 2020, 13:13 skadio, notifications@github.com wrote:

A quick update: I looked at the submissions but haven't completed the list yet, hoping to get back to it this weekend.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-718743658, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSQ4T7TTFBWWU3H73ETSNFTAFANCNFSM4Q74WFHQ .

prw99r commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

danielskatz commented 4 years ago

A quick update: I looked at the submissions but haven't completed the list yet, hoping to get back to it this weekend.

@skadio - any update? (I know weekends aren't great for things like this, but just checking...)

danielskatz commented 3 years ago

πŸ‘‹ @prw99r & @skadio - any updates from you? This review has been going about 2 months, and it would be really good to finish it soon.

I see a bunch of boxes checked, but some not checked. For each of you, are the unchecked boxes thing that you just haven't been able to check on yet, or are they items where there is work needed by the submitter, or discussion needed? If so, please say what these blocking issues are.

Otherwise, can you tell me when you think you will be done with your reviews?

prw99r commented 3 years ago

I've raised a couple of issues with the authors - I hope I'm not being too picky, but trying to maintain a high standard. I'm not satisfied yet. Peter

On Mon, Nov 9, 2020 at 1:17 PM Daniel S. Katz notifications@github.com wrote:

πŸ‘‹ @prw99r https://github.com/prw99r & @skadio https://github.com/skadio - any updates from you? This review has been going about 2 months, and it would be really good to finish it soon.

I see a bunch of boxes checked, but some not checked. For each of you, are the unchecked boxes thing that you just haven't been able to check on yet, or are they items where there is work needed by the submitter, or discussion needed? If so, please say what these blocking issues are.

Otherwise, can you tell me when you think you will be done with your reviews?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-724005980, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSURYMDGIASCF655DK3SO7TWFANCNFSM4Q74WFHQ .

danielskatz commented 3 years ago

Thanks @prw99r - if you can put https://github.com/openjournals/joss-reviews/issues/2646 in a comment in those issues, then they become visible here and I can see if they are open or closed, which helps me track how this review is going.

prw99r commented 3 years ago

Daniel, I've had a think, and I don't think I can approve this for JOSS publication in its current form. I took another look at the criteria for publication and guidelines and I just dont think it satisfies the criteria laid down - it is an OK bit of coding, as far as it goes, but a major scholarly contribution - no.

I'm not happy about the statement of need, references to prior work, and also the level of contribution - I just don't see it.

Some useful packages - sure, but major contribution - nope.

It is not about raising specific issues either, that misses the point. it is the fundamental contribution being asserted - I am not convinced it is particularly interesting or useful other than to a very limited audience - hence my comments about the statement of need.

Hope that helps, Peter

On Mon, Nov 9, 2020 at 1:17 PM Daniel S. Katz notifications@github.com wrote:

πŸ‘‹ @prw99r https://github.com/prw99r & @skadio https://github.com/skadio - any updates from you? This review has been going about 2 months, and it would be really good to finish it soon.

I see a bunch of boxes checked, but some not checked. For each of you, are the unchecked boxes thing that you just haven't been able to check on yet, or are they items where there is work needed by the submitter, or discussion needed? If so, please say what these blocking issues are.

Otherwise, can you tell me when you think you will be done with your reviews?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/2646#issuecomment-724005980, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESJLSURYMDGIASCF655DK3SO7TWFANCNFSM4Q74WFHQ .

danielskatz commented 3 years ago

Thanks @prw99r - I don't mean to minimize your concerns; I'm just trying to see where you have raised these issues and what the discussion has been on them.

danielskatz commented 3 years ago

Additionally, I wonder if @r-a-hoggarth has any thoughts on these issues.

skadio commented 3 years ago

@danielskatz I completed this round of my review. My checklist is updated accordingly and some comments are added below for our authors @jpsety

I hear and share some of the valid points raised by @prw99r. At the moment, I am a bit on the fence with this submission.

Overall, my understanding is that (please correct me if I am wrong) the library provides some I/O related capabilities to read/write circuit instances, an API to create/transform/interact with circuits, ability to do SAT queries.

Under the hood, the bulk of the work is done by the networkx library to hold on to that structure and the pysat library to answer SAT questions. The standardization/synthesis/analysis are nice contributions but I am not yet convinced these grant an acceptance at JOSS.

On the positive side, the package also ships with some circuit instances which others might find useful (assuming these are interesting instances for the community and are not available in standard formats elsewhere) and the library has the potential to "standardize" circuit operations to some extent. In that sense, one of the major contributions this library can have is "reproducibility" and sharing some common circuit formats --which can accelerate research.

That said, as the authors rightly noted in their submission as well, the library is geared toward benefit the originating lab in the first place. I am not in a good position to fairly judge the applicability/interest from the rest of the community for this tool and/or whether these are gaps are not at all supported by other existing work in this field.

===

danielskatz commented 3 years ago

πŸ‘‹ @jpsety - Can I get your response to the comments from @prw99r & @skadio?

danielskatz commented 3 years ago

πŸ‘‹ @jpsety - Can I get your response to the comments from @prw99r & @skadio?

jpsety commented 3 years ago

@skadio, Thanks for the feedback, I've addressed most of your comments, but have some further questions/comments.

Comment about the codebase: while it seems okay overall but there are some dangling imports, typos, inconsistencies with PEP8 (or just linters in general). The source code can benefit from a quick clean up.

I removed the unused imports/some variables I found. Ran the code through the Black linter as specified in our contribution policy. Fixed typos.

Codecov and coverage are listed as requirements, but I am not sure if there are necessary strict requirements. The setup.py does not list them either as install requirements so please consider simplifying the installation (or add them as required in setup.py)

Removed these from the requirements.txt, but this has broken our CI testing. Will find a fix.

Please explain what's fanin and fanout for the general reader. It is used in the Composition example without an explanation.

Added

I couldn't find a contributor policy?

We have guidelines on the readme of the repo.

Have you considered load/save capabilities of Circuit objects? If researchers would like to share or persist the state of their circuit objects if saving/load to bytecode (say pickle files) conforms the integrity. This might be good to check/add.

I opened an issue for testing the pickle interface. This is certainly a convenient addition. I believe it should work already, but I'll add some unit tests to be sure.

Under the hood, the bulk of the work is done by the networkx library to hold on to that structure and the pysat library to answer SAT questions

I think this characterization misses the large amount of work in converting between formats, adding interfaces to generate circuits, transforms that enable SAT calls, etc.

As per the JOSS guidelines:

Your software should be a significant contribution to the available open source software that either enables some new research challenges to be addressed or makes addressing research challenges significantly better (e.g., faster, easier, simpler).

This library makes working with Boolean circuits significantly easier. In only a few lines of code, a user can manipulate circuits in ways that otherwise would take several languages, tools, etc. See the comment here: comment

jpsety commented 3 years ago

@prw99r, I appreciate your feedback. You bring up good points in what can be added. I don't currently have the bandwidth to further edit this paper. As I understand it, the idea of the paper is to give a concise summary of the purpose of the library and show examples of the functionality. I have done that using other accepted projects as examples. I am confident that the targeted users of this library will be able to use it as is.

danielskatz commented 3 years ago

πŸ‘‹ @prw99r & @skadio - what are your thoughts on @jpsety's responses?

skadio commented 3 years ago

@danielskatz

First of all, I would like to thank @jpsety for the updates and improvements. Most of these comments were not immediate blockers for me but I shared with you in a positive spirit to improve the work for the next/intended users beyond the lead developer. It's nice to see that some are incorporated and others can be added as well.

Without going into a discussion of assessing the exact amount of work involved here, the author highlights an important aspect that I also agree with: "the large amount of work in converting between formats, adding interfaces to generate circuits, transforms that enable SAT calls".

I consider this as a valuable contribution since it can help bring consistency across different research applications and for "reproducibility" purposes. In light of that, I don't see an immediate reason why should the research community be barred from learning about this tool and benefit from its functionality. Hence, I would be happy to accept this submission with minor edits from the list above, if any remaining.

Best, Serdar